Release/d#26
Merged
magmacomputing merged 30 commits intomainfrom May 6, 2026
Merged
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCentralizes alias registration/resolution into a collision-aware AliasEngine, integrates host-based alias evaluation into the parser, adds runtime logging utilities/wiring, updates parsing patterns/composer/lexer behavior, migrates term plugin activation, updates tests, and refreshes docs and VitePress config for v2.9.0. ChangesAlias Engine + Parser Integration
Sequence DiagramsequenceDiagram
actor User
participant Tempo
participant Parser as DiscreteParser
participant AliasEngine
participant Host as ResolutionHost
participant Temporal
User->>Tempo: parse(input)
Tempo->>Parser: parse(value, state)
Parser->>Parser: parseGroups()
Parser->>Host: getResolutionContext(state, zdt)
Host-->>Parser: host API (add/subtract/with/set/toNow/toDateTime,...)
Parser->>AliasEngine: resolveAlias(key)
AliasEngine->>AliasEngine: traverse proto-chain (local → parent ...)
AliasEngine-->>Parser: AliasResult { value, key, type, source, isClock, isFunction }
alt AliasResult.isFunction
Parser->>Host: call function with host
Host->>Temporal: perform temporal ops
Temporal-->>Host: ZonedDateTime
Host-->>Parser: updated dateTime
else static/string
Parser->>Parser: re-parse(resolved value) with anchor propagation
end
Parser-->>Tempo: composed ZonedDateTime/timeZone
Tempo-->>User: result
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@copilot resolve the merge conflicts in this pull request |
… version Co-authored-by: magmacomputing <6935496+magmacomputing@users.noreply.github.com>
… version Agent-Logs-Url: https://github.com/magmacomputing/magma/sessions/984a79e1-3587-4d46-a02c-2e09df728e9e Co-authored-by: magmacomputing <6935496+magmacomputing@users.noreply.github.com>
Contributor
Merge conflicts resolved in commit
Build and all 12 alias engine tests pass after the merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
packages/tempo/src/tempo.class.ts (2)
640-658: 💤 Low valueHoist
Tempo.MONTH.keys()out of the inner loop and reuse the imported$Internalsymbol.Two minor cleanups in the term→alias sync block:
Tempo.MONTH.keys()is recomputed for every range entry. Hoist once before theforEach.- Line 653 uses
(this as any)[sym.$Internal](), but$Internalis already imported directly at the top of the file (line 29). Consistency with the rest of the class would prefer(this as any)[$Internal]().♻️ Proposed cleanup
if (type) { const aliases: [string, any][] = []; + const monthKeys = Tempo.MONTH.keys(); config.ranges.forEach(r => { if (r.key) { - const val = isDefined(r.hour) ? `${r.hour}:${pad(r.minute ?? 0)}` : (r.month ? `${pad(r.day ?? 1)} ${Tempo.MONTH.keys()[r.month - 1]}` : undefined); + const val = isDefined(r.hour) + ? `${r.hour}:${pad(r.minute ?? 0)}` + : (r.month ? `${pad(r.day ?? 1)} ${monthKeys[r.month - 1]}` : undefined); if (val) aliases.push([r.key, val]); } }); if (aliases.length > 0) { - const state = (this as any)[sym.$Internal](); + const state = (this as any)[$Internal](); if (type === 'per') (this as any)[$setPeriods](state, aliases); else if (type === 'evt') (this as any)[$setEvents](state, aliases); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/tempo.class.ts` around lines 640 - 658, Hoist Tempo.MONTH.keys() into a local variable before iterating config.ranges and replace the per-iteration call with that variable (e.g., const monthKeys = Tempo.MONTH.keys()) to avoid recomputing; also replace the use of (this as any)[sym.$Internal]() with the directly imported symbol (this as any)[$Internal]() so the internal state call matches the rest of the class, keeping the rest of the logic that builds aliases and calls $setPeriods/$setEvents unchanged.
144-264: ⚡ Quick winExtract shared logic from
$setEvents/$setPeriodsand collapse the duplicated engine-creation branch.Both methods are near-identical: the only differences are the field (
eventvsperiod), alias type ('evt'vs'per'), and snippet token (Token.evtvsToken.per). Future changes to inheritance handling, engine instantiation, or snippet wiring must be applied in lockstep, which is a maintenance hazard.Additionally, within each method the engine instantiation appears twice with identical constructor arguments (lines 168-174 then 177-183, and lines 231-237 then 240-246). The first guard handles "we have local aliases", the second handles "no engine inherited at all", but they construct the same object — they can be merged into one branch on
!hasOwn(shape, 'aliasEngine')once you've decided you need a local engine.♻️ Sketch: shared helper + single engine-creation branch
type AliasField = 'event' | 'period'; type AliasKind = 'evt' | 'per'; private static _syncAliases( shape: Internal.State, field: AliasField, kind: AliasKind, token: symbol, provided?: [string, any][], ) { const parent = proto(shape); const parentMap = parent.parse?.[field] ?? {}; const entries = provided ?? ownEntries(shape.parse[field], true).filter(([k]) => { return !(k in parentMap) || shape.parse[field][k as string] !== parentMap[k as string]; }); if (provided) { provided.forEach(([k, v]) => { if (!hasOwn(shape.parse[field], k)) shape.parse[field][k as string] = v; }); } if (entries.length === 0 && !hasOwn(shape, 'aliasEngine')) return; // Single branch for engine ownership if (!hasOwn(shape, 'aliasEngine')) { shape.aliasEngine = new AliasEngine({ parent: parent.aliasEngine, logger: Tempo.#dbg, config: shape.config, }); } const engine = shape.aliasEngine!; const groups = engine.registerAliases(kind, entries); const protoSrc = parent.parse?.snippet?.[token]?.source; if (groups) { if (groups !== protoSrc) { if (!hasOwn(shape.parse, 'snippet')) shape.parse.snippet = { ...shape.parse.snippet }; setProperty(shape.parse.snippet, token, new RegExp(groups)); } } else if (hasOwn(shape.parse.snippet, token)) { delete shape.parse.snippet[token as any]; } } static [$setEvents](shape: Internal.State, provided?: [string, any][]) { return Tempo._syncAliases(shape, 'event', 'evt', Token.evt, provided); } static [$setPeriods](shape: Internal.State, provided?: [string, any][]) { return Tempo._syncAliases(shape, 'period', 'per', Token.per, provided); }Also note the
else { if (hasOwn(shape.parse.snippet, token)) ... }branch is reached whengroupsis''(empty string from[].join('|')) as well asundefined. That's likely intended — just worth confirming the empty-pattern path is desired when an engine has no own or inherited aliases of that kind.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/tempo.class.ts` around lines 144 - 264, Both $setEvents and $setPeriods duplicate the same alias registration logic (field, kind, token differ) and instantiate AliasEngine twice; extract a shared helper (e.g. Tempo._syncAliases) that accepts parameters (field: 'event'|'period', kind: 'evt'|'per', token: Token, provided?: [string, any][]) and implements: compute parent map, compute entries (provided or filtered ownEntries), sync provided into shape.parse[field], return early if no entries and no local aliasEngine, then create a local aliasEngine in a single branch when !hasOwn(shape, 'aliasEngine') (using parent.aliasEngine, Tempo.#dbg, shape.config), call engine.registerAliases(kind, entries), set or delete shape.parse.snippet[token] based on groups (preserving the current proto comparison logic), and finally replace $setEvents/$setPeriods to call this helper with the proper arguments.packages/tempo/src/discrete/discrete.parse.ts (2)
388-409: 💤 Low valueAvoid double
getAliaslookup per group key.
aliasEngine.getAlias(key)is called explicitly at line 393, and thenaliasEngine.resolveAlias(key, host)is called at line 408 which internally invokesgetAlias(key)again (seeengine.alias.ts:178). For groups with many keys this is redundant work; you can readregister.namefrom the resolved result instead.♻️ Sketch: resolve once and reuse
- const register = aliasEngine?.getAlias(key); - if (!register) continue; - - const aliasKey = register.name; - if (resolvingKeys.size > 50 || resolvingKeys.has(aliasKey)) { + const host = getResolutionContext(state, dateTime, resolvingKeys); + const res = aliasEngine?.resolveAlias(key as any, host); + if (!res) continue; + + const aliasKey = res.key; + if (resolvingKeys.size > 50 || resolvingKeys.has(aliasKey)) { const msg = `Infinite recursion detected in Tempo resolution for: ${aliasKey}`; state.errored = true; if (TempoClass) (TempoClass as any)[sym.$logError](state.config, new RangeError(msg)); delete groups[key]; continue; } resolvingKeys.add(aliasKey); - - const host = getResolutionContext(state, dateTime, resolvingKeys); - const res = aliasEngine?.resolveAlias(key as any, host); - if (!res) continue;Note that
resolveAliaswill invoke any function-typed alias target withhostasthisArg, even if we later discover recursion and skip — verify that calling those handlers eagerly is acceptable, or fall back to a cheapgetAliaspre-check that avoids invocation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/discrete/discrete.parse.ts` around lines 388 - 409, Currently the code calls aliasEngine.getAlias(key) into register and later calls aliasEngine.resolveAlias(key, host) which internally re-calls getAlias; change the flow to call aliasEngine.resolveAlias(key, host) once, reuse the resolved object's alias metadata (e.g., resolved.register or resolved.name) instead of calling getAlias again, and use that alias name for recursion checks (resolvingKeys and aliasKey) and error reporting (state, TempoClass[sym.$logError]). If eager invocation of function-typed alias targets is a concern, switch to a cheap pre-check using getAlias(key) to obtain only the register/name before calling resolveAlias, so you avoid invoking handlers when detecting recursion.
411-433: 💤 Low valueUse exhaustive switch or lookup table instead of ternary to catch future AliasType additions at compile time.
The ternary at lines 412–413 (
res.type === 'evt' ? 'Event' : 'Period') silently defaults toPeriod/tmfor any value other than'evt'. SinceAliasType = 'evt' | 'per'is a closed union, adding a new type (e.g.,'dur') will not cause a compile-time error here—it will just be misclassified. An exhaustive switch or lookup table would catch this immediately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/discrete/discrete.parse.ts` around lines 411 - 433, The ternary mapping of res.type to type/pat (res.type === 'evt' ? 'Event' : 'Period' and similarly for pat) can silently misclassify new AliasType variants; replace this with an exhaustive switch or a lookup object keyed by AliasType (e.g., mapping 'evt' -> { type: 'Event', pat: 'dt' }, 'per' -> { type: 'Period', pat: 'tm' }) used before calling _ParseEngine.result, and ensure the default branch throws or asserts on unexpected res.type so new AliasType members cause a compile-time/explicit failure rather than silent fallback; update all uses of the local variables type and pat accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tempo/.vitepress/config.ts`:
- Line 16: The sidebar contains a link to '/CONTRIBUTING' but the VitePress
config's srcExclude array (symbol: srcExclude) currently excludes
'CONTRIBUTING.md', causing a dead docs link; fix by either removing
'CONTRIBUTING.md' from srcExclude so the file is included in the build or by
updating the sidebar entry that references '/CONTRIBUTING' (look for the
sidebar/menu configuration where '/CONTRIBUTING' is referenced) to point to an
existing doc or remove the link entirely; ensure the change is made to the same
config.ts so the sidebar and srcExclude are consistent.
In `@packages/tempo/src/engine/engine.alias.ts`:
- Around line 220-229: The clear(type: AliasType) method removes entries from
this.#state and resets this.#count[type] but fails to update this.#words,
leaving stale collision info; after deleting state entries for the given
AliasType you must rebuild this.#words (or remove only the words belonging to
deleted aliases) from the remaining this.#state and ensure this.#count is
consistent before incrementing this.#version; update the clear function to
iterate remaining this.#state to reconstruct this.#words (and adjust this.#count
if you prefer recomputing counts from state) so re-registering the same alias
won't be treated as an immediate collision.
- Around line 117-122: getPatterns() must honor the collision metadata set in
registerAliases() instead of only deduplicating by requested type: when
assembling pattern lists in getPatterns(), read the collision flag stored on
entries in this.#words (the same metadata used where `const collision = baseWord
in this.#words` is set) and exclude any base-word entries that lost the
collision according to the documented policy (e.g., if an event alias won, do
not include the same base word in period/"per" patterns). Update both places
noted (the getPatterns() logic around lines referenced and the duplicate block
at 145-162) to filter out entries where entry.collision/marker indicates they
were overridden by the other type so parsing follows the event-vs-period
priority. Ensure you reference this.#words and the collision marker when
deciding which aliases to include.
In `@packages/tempo/src/engine/engine.composer.ts`:
- Around line 77-80: After logging the invalid number in the Number branch, stop
further processing so NaN/Infinity cannot fall through into the seconds-parsing
path and overwrite the intended fallback; immediately return (or otherwise exit
the surrounding function/control flow) after the logError(config, `Invalid Tempo
number: ${value}`) and setting temporal = today so execution never continues
into the subsequent seconds-parsing code that uses the same temporal variable.
In `@packages/tempo/src/support/tempo.init.ts`:
- Around line 39-42: The code currently uses
Object.create(baseState.parse.monthDay) and
Object.create(baseState.parse.planner), which shares prototypes and allows
in-place mutations of nested fields (e.g., layoutOrder, locales, timezones,
resolvedLocales) to leak into baseState; replace those Object.create usages with
proper deep/shallow copies instead: for monthDay call resolveMonthDay with a
copied source (or structuredClone) so nested arrays/objects are new instances,
and for planner build a new object that copies primitives and clones
arrays/objects returned by asArray(Default.planner?.layoutOrder) and any
Default.planner nested structures (ensure layoutOrder, preFilter, locales,
timezones, resolvedLocales are cloned), avoiding prototype sharing with
baseState.parse and preserving immutability of the global state.
---
Nitpick comments:
In `@packages/tempo/src/discrete/discrete.parse.ts`:
- Around line 388-409: Currently the code calls aliasEngine.getAlias(key) into
register and later calls aliasEngine.resolveAlias(key, host) which internally
re-calls getAlias; change the flow to call aliasEngine.resolveAlias(key, host)
once, reuse the resolved object's alias metadata (e.g., resolved.register or
resolved.name) instead of calling getAlias again, and use that alias name for
recursion checks (resolvingKeys and aliasKey) and error reporting (state,
TempoClass[sym.$logError]). If eager invocation of function-typed alias targets
is a concern, switch to a cheap pre-check using getAlias(key) to obtain only the
register/name before calling resolveAlias, so you avoid invoking handlers when
detecting recursion.
- Around line 411-433: The ternary mapping of res.type to type/pat (res.type ===
'evt' ? 'Event' : 'Period' and similarly for pat) can silently misclassify new
AliasType variants; replace this with an exhaustive switch or a lookup object
keyed by AliasType (e.g., mapping 'evt' -> { type: 'Event', pat: 'dt' }, 'per'
-> { type: 'Period', pat: 'tm' }) used before calling _ParseEngine.result, and
ensure the default branch throws or asserts on unexpected res.type so new
AliasType members cause a compile-time/explicit failure rather than silent
fallback; update all uses of the local variables type and pat accordingly.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 640-658: Hoist Tempo.MONTH.keys() into a local variable before
iterating config.ranges and replace the per-iteration call with that variable
(e.g., const monthKeys = Tempo.MONTH.keys()) to avoid recomputing; also replace
the use of (this as any)[sym.$Internal]() with the directly imported symbol
(this as any)[$Internal]() so the internal state call matches the rest of the
class, keeping the rest of the logic that builds aliases and calls
$setPeriods/$setEvents unchanged.
- Around line 144-264: Both $setEvents and $setPeriods duplicate the same alias
registration logic (field, kind, token differ) and instantiate AliasEngine
twice; extract a shared helper (e.g. Tempo._syncAliases) that accepts parameters
(field: 'event'|'period', kind: 'evt'|'per', token: Token, provided?: [string,
any][]) and implements: compute parent map, compute entries (provided or
filtered ownEntries), sync provided into shape.parse[field], return early if no
entries and no local aliasEngine, then create a local aliasEngine in a single
branch when !hasOwn(shape, 'aliasEngine') (using parent.aliasEngine, Tempo.#dbg,
shape.config), call engine.registerAliases(kind, entries), set or delete
shape.parse.snippet[token] based on groups (preserving the current proto
comparison logic), and finally replace $setEvents/$setPeriods to call this
helper with the proper arguments.
🪄 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: 58dbddad-18ec-4346-af49-2da42c72ea59
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
packages/tempo/.vitepress/config.tspackages/tempo/plan/alias-migration-phase2.mdpackages/tempo/plan/alias.registration.mdpackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/engine/engine.alias.tspackages/tempo/src/engine/engine.composer.tspackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/tempo.default.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/support/tempo.runtime.tspackages/tempo/src/support/tempo.util.tspackages/tempo/src/tempo.class.tspackages/tempo/test/core/alias-engine-protochain.test.tspackages/tempo/test/core/alias-engine.mock.test.tspackages/tempo/test/core/alias-engine.test.tspackages/tempo/test/core/constructor.core.test.tspackages/tempo/test/core/tempo_guard.test.tspackages/tempo/test/engine/parse.prefilter.numeric-safety.test.tspackages/tempo/test/plugins/ticker.patterns.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/tempo/src/engine/engine.alias.ts (1)
236-238: 💤 Low valueConsider removing the
as anycast for cleaner type safety.The cast
(this.#parent as any).#wordsis needed because TypeScript's strict private field access through a nullable reference. While functionally correct, consider using a getter method onAliasEngineto expose the parent's#wordsfor cleaner access:+ protected get _words() { return this.#words; } + clear(type: AliasType) { // ... - this.#words = Object.create(this.#parent ? (this.#parent as any).#words : null); + this.#words = Object.create(this.#parent ? this.#parent._words : null);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.alias.ts` around lines 236 - 238, The code uses a type-unsafe cast (this.#parent as any).#words to access a private field; add a safe accessor on AliasEngine (e.g., a getter method like getWords() that returns the parent's words shape) and use this.#parent?.getWords() (or the equivalent getter) when constructing this.#words instead of the as any cast; update references to `#words` access from parent to use that getter so the private field is not type-violently accessed.packages/tempo/src/tempo.class.ts (2)
1454-1473: 💤 Low valueDead local in
#setLocal.
const self = unwrap(this)is declared but never read in the method body — the subsequent code usesthis.#localandthisdirectly. Either drop the binding, or use it where a proxy-unwrapped reference is genuinely required (e.g. fortempoInstance: thisif you intended to store the unwrapped instance there).♻️ Proposed cleanup
this.#local = Object.create(classState); (this.#local as any)._id = (this.constructor as any)[$Internal]()._count++; - const self = unwrap(this); this.#local.config = markConfig(Object.create(classState.config));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/tempo.class.ts` around lines 1454 - 1473, The local binding const self = unwrap(this) in `#setLocal` is unused; either remove that line or use the unwrapped reference when linking the instance—i.e., in the Object.defineProperty for 'tempoInstance' assign the unwrapped self instead of this if you intended to store a proxy-free instance; otherwise simply delete the unused const self = unwrap(this) to eliminate the dead local. Ensure references to unwrap, `#local`, tempoInstance and `#setLocal` are updated consistently.
192-199: 💤 Low valueAddress (or convert to issue) the alias/Layout sync TODO.
{evt}/{per}snippets are kept fresh, but Layouts that already inline{evt}/{per}placeholders won't be regenerated after[$setEvents]/[$setPeriods]runs, which can leave stale parse RegExps until the nextsetPatterns. Worth tracking explicitly.Want me to open a GitHub issue describing the Layout-rebuild requirement so this TODO can be tracked outside the source?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/tempo.class.ts` around lines 192 - 199, The TODO notes that inline Layouts containing "{evt}" or "{per}" won't be regenerated by [$setEvents]/[$setPeriods], so update these methods (which call [$setAliases] with Token.evt/Token.per) to mark or rebuild affected Layouts: after invoking (this as any)[$setAliases](...), scan the shape.Layouts (or use an existing rebuild/setPatterns helper) for any layout text containing "{evt}" or "{per}" and either enqueue them for regeneration or set a dirty flag that setPatterns will pick up; reference $setEvents, $setPeriods, $setAliases, Token.evt, Token.per and the setPatterns/layout regeneration path when implementing the change so parse RegExps are refreshed immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tempo/src/tempo.class.ts`:
- Around line 153-189: Local Tempo instances can accidentally mutate parent
parse state because shape.parse.event/period/snippet may be inherited via
prototype; before writing to (shape.parse[field] as any)[k] or deleting
(shape.parse.snippet as any)[token], ensure a clone-on-write so the shape has
its own own-property copy. In `#setLocal` and in the $setAliases flow (look for
provided, shape.parse[field], shape.parse.snippet, registerAliases,
setProperty), guard mutations by: if (!hasOwn(shape.parse, field)) clone
shape.parse[field] = { ...shape.parse[field] } before assigning keys, and
similarly if (!hasOwn(shape.parse, 'snippet')) clone shape.parse.snippet = {
...shape.parse.snippet } before deleting keys; alternatively shadow
event/period/snippet in `#setLocal` like planner/monthDay to ensure writes go to
the local object.
- Around line 178-184: The regex is built from user-controlled alias strings
without escaping, so replace the direct use of register.name when constructing
patterns in engine.alias.ts (the code that does
patterns.push(`(?<${alias}>${register.name})`)) with an escaped version using
the existing Match.escape utility; update the pattern construction in the
registerAliases-related code path so it calls Match.escape(register.name) before
interpolation to prevent regex metacharacter interpretation and pathological
backtracking.
---
Nitpick comments:
In `@packages/tempo/src/engine/engine.alias.ts`:
- Around line 236-238: The code uses a type-unsafe cast (this.#parent as
any).#words to access a private field; add a safe accessor on AliasEngine (e.g.,
a getter method like getWords() that returns the parent's words shape) and use
this.#parent?.getWords() (or the equivalent getter) when constructing
this.#words instead of the as any cast; update references to `#words` access from
parent to use that getter so the private field is not type-violently accessed.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 1454-1473: The local binding const self = unwrap(this) in
`#setLocal` is unused; either remove that line or use the unwrapped reference when
linking the instance—i.e., in the Object.defineProperty for 'tempoInstance'
assign the unwrapped self instead of this if you intended to store a proxy-free
instance; otherwise simply delete the unused const self = unwrap(this) to
eliminate the dead local. Ensure references to unwrap, `#local`, tempoInstance and
`#setLocal` are updated consistently.
- Around line 192-199: The TODO notes that inline Layouts containing "{evt}" or
"{per}" won't be regenerated by [$setEvents]/[$setPeriods], so update these
methods (which call [$setAliases] with Token.evt/Token.per) to mark or rebuild
affected Layouts: after invoking (this as any)[$setAliases](...), scan the
shape.Layouts (or use an existing rebuild/setPatterns helper) for any layout
text containing "{evt}" or "{per}" and either enqueue them for regeneration or
set a dirty flag that setPatterns will pick up; reference $setEvents,
$setPeriods, $setAliases, Token.evt, Token.per and the setPatterns/layout
regeneration path when implementing the change so parse RegExps are refreshed
immediately.
🪄 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: c6ff5e0c-b2a3-4617-a144-4b2c73a8fa99
📒 Files selected for processing (23)
packages/tempo/.vitepress/config.tspackages/tempo/CHANGELOG.mdpackages/tempo/README.mdpackages/tempo/doc/architecture.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/doc/tempo.modularity.mdpackages/tempo/doc/tempo.parse.mdpackages/tempo/doc/tempo.term.mdpackages/tempo/package.jsonpackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/engine/engine.alias.tspackages/tempo/src/engine/engine.composer.tspackages/tempo/src/plugin/term.util.tspackages/tempo/src/plugin/term/standard.index.tspackages/tempo/src/plugin/term/term.index.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/support/tempo.symbol.tspackages/tempo/src/tempo.class.tspackages/tempo/test/plugins/fiscal-cycle.core.test.tspackages/tempo/test/plugins/term-dispatch.core.test.tspackages/tempo/test/plugins/ticker.term.core.test.tspackages/tempo/vitest.config.ts
💤 Files with no reviewable changes (2)
- packages/tempo/src/plugin/term/standard.index.ts
- packages/tempo/vitest.config.ts
✅ Files skipped from review due to trivial changes (2)
- packages/tempo/CHANGELOG.md
- packages/tempo/test/plugins/ticker.term.core.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/tempo/src/engine/engine.alias.ts (1)
117-166:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe documented event-over-period collision rule is still order- and scope-dependent.
Line 124 always overwrites
#words[baseWord]with the last registration, so anevtregistered first and a collidingperregistered later makes the period look like the winner. Then Lines 164-166 recurse intoparent.getPatterns(), which evaluates collisions againstparent.#wordsand cannot see a childevtwinner. Both cases still let collidingper...groups leak into the parser.Preserve the effective winner in
#wordswith explicitevtprecedence, and pass that winner lookup through recursivegetPatterns()calls instead of recomputing it per ancestor frame.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.alias.ts` around lines 117 - 166, The collision handling currently overwrites this.#words[baseWord] with the last registration, allowing a later 'per' to win over an earlier 'evt' and also causing ancestors to recompute winners incorrectly; fix by preserving evt precedence when registering and by passing a consistent winner lookup through getPatterns recursion. Concretely: in the alias registration path (where collision is computed and this.#words[baseWord] = aliasKey and this.#state[aliasKey] = ...), change the assignment so that if an existing entry for baseWord maps to an alias whose this.#state[existingKey].type === 'evt', do not overwrite this.#words[baseWord] with a new 'per' alias (keep the existing evt winner); then change getPatterns(type, seenBaseNames = new Set()) to accept and use an explicit winnerLookup (e.g., wordsLookup = this.#words or a shallow copy built at the child level) instead of reading this.#words inside ancestor frames (replace uses of this.#words[register.baseWord] with winnerLookup[register.baseWord]) and pass the same winnerLookup when recursing to this.#parent.getPatterns(...), ensuring the same effective winner resolution (evt-over-per) is applied across the full recursion.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
41-41: ⚡ Quick winSimplify duplicated PR branch gating to avoid drift
Line 41 repeats branch filtering already enforced by
on.pull_request.branches(Lines 14-16). Keeping both can silently diverge later. Consider reducing this to PR-only gating and letting the trigger own branch selection.Suggested simplification
- if: github.event_name == 'pull_request' && (github.event.pull_request.base.ref == 'main' || github.event.pull_request.base.ref == 'release-c-layout-order-planner') + if: github.event_name == 'pull_request'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 41, The workflow currently duplicates PR branch filtering by using the if condition "github.event_name == 'pull_request' && (github.event.pull_request.base.ref == 'main' || github.event.pull_request.base.ref == 'release-c-layout-order-planner')" while branch selection is already declared under on.pull_request.branches; simplify by changing the job-level if to only check the event (e.g., "github.event_name == 'pull_request'") and rely on the existing on.pull_request.branches configuration to enforce which branches run, updating any job-level "if" that references github.event.pull_request.base.ref accordingly.packages/tempo/src/support/tempo.default.ts (1)
205-211: 💤 Low valueConfirm intentional behavior change:
Default.planneris effectively{}.All example fields are commented out, so
Default.planner.layoutOrderisundefinedand the legacy(Default as any).layoutOrderfallback intempo.init.ts(L52-53) also resolves toundefined. Net effect for fresh (non-baseState) inits:layoutOrderbecomes whateverasArray(undefined)returns andpreFilterbecomesfalse. Per the AI summary this is intentional ("widened surface without enabling new defaults"), but worth a quick sanity check that downstream parser code tolerates an empty/absentlayoutOrder.If you want the example block to remain self-documenting without changing behavior, consider moving the comment out of the literal so readers don't mistake it for a default that is somehow active.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/support/tempo.default.ts` around lines 205 - 211, Default.planner currently is an empty object because the example fields are commented out, causing Default.planner.layoutOrder to be undefined and the legacy fallback ((Default as any).layoutOrder in tempo.init.ts) to also yield undefined; to avoid accidental behavior change or reader confusion either restore the intended defaults (set planner.layoutOrder and planner.preFilter explicitly on Default.planner) or move the example block out of the object literal into a nearby comment/docstring so the file documents the options without making Default.planner appear empty—update references to Default.planner and tempo.init.ts accordingly to ensure downstream code that uses layoutOrder still gets the intended value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tempo/src/engine/engine.alias.ts`:
- Around line 88-95: The constructor currently logs an invalid this.#parent but
continues and dereferences it; update the AliasEngine constructor to validate
that this.#parent (used when setting this.#depth, this.#state, this.#words) is
an instance of AliasEngine and early-return or throw immediately after logging
when it's not; specifically, in the block that checks this.#parent, move the
instanceof check to guard the subsequent uses of this.#parent (or add an
explicit if (! (this.#parent instanceof AliasEngine)) {
this.#logger?.error(this.#config, "Parent engine must be an instance of
AliasEngine"); return/throw } ) so that `#depth`, `#state` and `#words` are only
accessed when this.#parent is a valid AliasEngine instance.
---
Duplicate comments:
In `@packages/tempo/src/engine/engine.alias.ts`:
- Around line 117-166: The collision handling currently overwrites
this.#words[baseWord] with the last registration, allowing a later 'per' to win
over an earlier 'evt' and also causing ancestors to recompute winners
incorrectly; fix by preserving evt precedence when registering and by passing a
consistent winner lookup through getPatterns recursion. Concretely: in the alias
registration path (where collision is computed and this.#words[baseWord] =
aliasKey and this.#state[aliasKey] = ...), change the assignment so that if an
existing entry for baseWord maps to an alias whose this.#state[existingKey].type
=== 'evt', do not overwrite this.#words[baseWord] with a new 'per' alias (keep
the existing evt winner); then change getPatterns(type, seenBaseNames = new
Set()) to accept and use an explicit winnerLookup (e.g., wordsLookup =
this.#words or a shallow copy built at the child level) instead of reading
this.#words inside ancestor frames (replace uses of
this.#words[register.baseWord] with winnerLookup[register.baseWord]) and pass
the same winnerLookup when recursing to this.#parent.getPatterns(...), ensuring
the same effective winner resolution (evt-over-per) is applied across the full
recursion.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 41: The workflow currently duplicates PR branch filtering by using the if
condition "github.event_name == 'pull_request' &&
(github.event.pull_request.base.ref == 'main' ||
github.event.pull_request.base.ref == 'release-c-layout-order-planner')" while
branch selection is already declared under on.pull_request.branches; simplify by
changing the job-level if to only check the event (e.g., "github.event_name ==
'pull_request'") and rely on the existing on.pull_request.branches configuration
to enforce which branches run, updating any job-level "if" that references
github.event.pull_request.base.ref accordingly.
In `@packages/tempo/src/support/tempo.default.ts`:
- Around line 205-211: Default.planner currently is an empty object because the
example fields are commented out, causing Default.planner.layoutOrder to be
undefined and the legacy fallback ((Default as any).layoutOrder in
tempo.init.ts) to also yield undefined; to avoid accidental behavior change or
reader confusion either restore the intended defaults (set planner.layoutOrder
and planner.preFilter explicitly on Default.planner) or move the example block
out of the object literal into a nearby comment/docstring so the file documents
the options without making Default.planner appear empty—update references to
Default.planner and tempo.init.ts accordingly to ensure downstream code that
uses layoutOrder still gets the intended value.
🪄 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: 205c9394-affc-4d5d-a206-df133934e16c
📒 Files selected for processing (5)
.github/workflows/ci.ymlpackages/tempo/src/engine/engine.alias.tspackages/tempo/src/support/tempo.default.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/tempo.class.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tempo/src/tempo.class.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tempo/src/tempo.class.ts (1)
753-753:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale type cast on
layoutOrder;Options['parseOrder']does not exist.
PlannerOptions.layoutOrderis now(string | symbol)[](andDefault.planner.layoutOrderpopulates it withToken.*symbols), so casting throught.Options['parseOrder'](no such property onOptions) and finally tostring[]is both an incorrect type and a hidden lie about the runtime contents — symbols surviveasArray(...)and end up assigned to astring[]slot.🛡️ Suggested fix
- parse.planner.layoutOrder = asArray((Default.planner?.layoutOrder ?? (Default as any).layoutOrder) as t.Options['parseOrder']) as string[]; + parse.planner.layoutOrder = asArray<string | symbol>(Default.planner?.layoutOrder ?? (Default as any).layoutOrder);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/tempo.class.ts` at line 753, The code incorrectly casts layoutOrder through a non-existent t.Options['parseOrder'] and forces a string[] even though PlannerOptions.layoutOrder is (string | symbol)[]. Update the assignment for parse.planner.layoutOrder (the call that uses asArray((Default.planner?.layoutOrder ?? (Default as any).layoutOrder) ...)) to stop casting to t.Options['parseOrder'] and string[]; instead cast or type it as the real PlannerOptions.layoutOrder type (i.e., (string | symbol)[]) or use the PlannerOptions.layoutOrder type alias so symbols are preserved at runtime.
🧹 Nitpick comments (1)
packages/tempo/src/tempo.class.ts (1)
580-599: 💤 Low valueTerm-range → alias sync: edge cases worth a quick guard.
The derivation
r.month ? \${pad(r.day ?? 1)} ${monthKeys[r.month - 1]}`will silently produce"01 undefined"ifr.monthis out of range (e.g. accidentally13), which then registers as a malformed event alias. A simple bounds check (r.month >= 1 && r.month <= 12) avoids that without changing intended behavior. Same idea forr.hour(>= 0 && <= 23`) before formatting the period string.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/tempo.class.ts` around lines 580 - 599, Guard the month and hour bounds before building alias values so out-of-range inputs don't produce malformed aliases; when iterating config.ranges in the block that uses Tempo.MONTH.keys(), check r.month is an integer between 1 and 12 before using monthKeys[r.month - 1], and check r.hour is an integer between 0 and 23 before using `${r.hour}:${pad(...)}`; keep the existing flow that collects aliases and calls (this as any)[$Internal](), (this as any)[$setPeriods](...), and (this as any)[$setEvents](...) but skip/omit any range entries with invalid month/hour.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 41: The workflow's conditional on the prefilter job (the `if:`
expression) currently restricts runs to pushes only and references a stale
`release-c-layout-order-planner` branch; change the `if` on that job to include
`github.event_name == 'pull_request'` (or use `github.event_name == 'push' ||
github.event_name == 'pull_request'`) so PRs validate the prefilter benchmark,
and update or remove the stale branch name (replace
`release-c-layout-order-planner` with the current release branch, e.g.,
`release/D`, or drop the explicit second-branch check) to ensure correct branch
coverage.
In `@packages/tempo/src/engine/engine.alias.ts`:
- Around line 29-36: The AliasResult.key documentation and its runtime value
disagree: resolveAlias sets key to register.name (the registered alias pattern
like 'mid[ -]?night') but the comment calls it "the original baseName (e.g.
'noon')"; decide which you want and make them consistent — either (A) keep the
runtime behavior and change the AliasResult.key comment to say it contains the
registered alias pattern (reference: resolveAlias and register.name), or (B)
change resolveAlias to set key = register.baseWord so key truly holds the base
word (reference: register.baseWord); apply the same fix to the other AliasResult
declaration/use sites mentioned (the duplicate at the later block).
- Around line 187-193: hasAlias currently treats the input as an AliasKey but
callers pass user-facing alias names; update hasAlias(name: string, type?:
AliasType) to resolve the user name to the internal key before checking `#state`:
look up the base/normalized word via the existing `#words/getBaseWord` helper
(e.g. const key = this.#words?.[getBaseWord(name)] or call the module's
word-to-key resolver), then return false if no key, otherwise compare
this.#state[key as AliasKey].type to AliasType when provided; alternatively, if
you prefer a strict API change, change the parameter type to AliasKey and add a
separate hasName(name: string) that uses `#words/getBaseWord` to find the AliasKey
and delegate to hasAlias.
- Around line 131-142: The collision flag is computed after you overwrite
this.#words[baseWord], so on the overwrite path it will always be true; instead
use the previously captured snapshot (existing) to determine whether baseWord
already existed. Change the collision assignment in the state object (inside the
branch that sets this.#state[aliasKey]) to use the pre-check value (existing or
however the pre-existing entry was named) rather than evaluating baseWord in
this.#words after you possibly set it in the shouldOverwrite branch, ensuring
collision reflects the prior existence of baseWord.
---
Outside diff comments:
In `@packages/tempo/src/tempo.class.ts`:
- Line 753: The code incorrectly casts layoutOrder through a non-existent
t.Options['parseOrder'] and forces a string[] even though
PlannerOptions.layoutOrder is (string | symbol)[]. Update the assignment for
parse.planner.layoutOrder (the call that uses
asArray((Default.planner?.layoutOrder ?? (Default as any).layoutOrder) ...)) to
stop casting to t.Options['parseOrder'] and string[]; instead cast or type it as
the real PlannerOptions.layoutOrder type (i.e., (string | symbol)[]) or use the
PlannerOptions.layoutOrder type alias so symbols are preserved at runtime.
---
Nitpick comments:
In `@packages/tempo/src/tempo.class.ts`:
- Around line 580-599: Guard the month and hour bounds before building alias
values so out-of-range inputs don't produce malformed aliases; when iterating
config.ranges in the block that uses Tempo.MONTH.keys(), check r.month is an
integer between 1 and 12 before using monthKeys[r.month - 1], and check r.hour
is an integer between 0 and 23 before using `${r.hour}:${pad(...)}`; keep the
existing flow that collects aliases and calls (this as any)[$Internal](), (this
as any)[$setPeriods](...), and (this as any)[$setEvents](...) but skip/omit any
range entries with invalid month/hour.
🪄 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: e687a6be-c5e9-4c67-bffc-80f0270eb5e4
📒 Files selected for processing (7)
.github/workflows/ci.ymlpackages/tempo/src/engine/engine.alias.tspackages/tempo/src/parse/parse.layout.tspackages/tempo/src/support/tempo.default.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.ts
Owner
Author
|
@coderabbitai please review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
install the AliasEngine to manage all state related to Event|Period aliases
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores