Feature/tempo pattern compiler#28
Open
magmacomputing wants to merge 14 commits intomainfrom
Open
Conversation
…r improved state management and caching
|
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 (10)
📝 WalkthroughWalkthroughThis PR bumps versions to 2.9.2 and restructures Tempo internals: extracts pattern compilation into a per-state PatternCompiler, extracts guard logic into createMasterGuard, migrates many support modules from ChangesEngine & Support Extraction
Sequence DiagramsequenceDiagram
participant User as User Code
participant Tempo as Tempo Class
participant State as Internal State
participant Pattern as PatternCompiler
participant Guard as createMasterGuard
participant Engine as Engine Modules
User->>Tempo: new Tempo(...)
Tempo->>Tempo: `#buildGuard`()
Tempo->>Guard: createMasterGuard(wordsList)
Guard-->>Tempo: MasterGuard.test()
User->>Tempo: Tempo.regexp(layout)
Tempo->>State: get state.patternCompiler
alt not initialized
Tempo->>Pattern: new PatternCompiler(state)
Tempo->>State: store patternCompiler
end
Tempo->>Pattern: compileRegExp(layout)
Pattern->>Engine: resolve placeholders / layout
Pattern-->>Tempo: anchored RegExp
User->>Tempo: parse(input)
Tempo->>Guard: test(input)
Guard-->>Tempo: boolean
alt allowed
Tempo->>Engine: normalizeMatch / parse pipeline
Engine-->>Tempo: DateTime result
else denied
Tempo-->>User: reject / defer
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tempo/src/engine/engine.alias.ts (1)
83-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalid
parentis logged but still retained — downstream methods will crash.The constructor sets
this.#parent = options.parent ?? nullbefore theinstanceof AliasEnginecheck, and the new else branch only logs whenparentis truthy but not anAliasEngine. The invalid reference is still kept onthis.#parent, so later code that gates onif (this.#parent) { this.#parent.getPatterns(...) }(line 172–175),this.#parent?.getVersion()(line 181),this.#parent?.getAlias(key)(line 219), and(this.#parent as any).#words(line 247) will throwTypeErrors the next time aliases are registered/resolved. Previously this surfaced as an explicitTypeErrorat construction; now it only surfaces deep inside parsing.Either restore the strict throw or null out the parent in the invalid branch so root semantics are actually root semantics.
🛡️ Proposed fix — null out invalid parent so root paths stay root
if (this.#parent instanceof AliasEngine) { this.#depth = this.#parent.#depth + 1; this.#state = Object.create(this.#parent.#state); // create a new state object that inherits from the parent engine's state this.#words = Object.create(this.#parent.#words); // create a new words object that inherits from the parent engine's words for collision detection } else { - if (this.#parent) - this.#logger?.error(this.#config, "Parent engine must be an instance of AliasEngine"); - + if (this.#parent) { + this.#logger?.error(this.#config, "Parent engine must be an instance of AliasEngine"); + this.#parent = null; // avoid retaining an invalid reference that downstream methods would dereference + } this.#depth = 0; this.#state = Object.create(null); // initialize an empty state for the root engine (no parent) this.#words = Object.create(null); // initialize an empty words object for the root engine (no parent) }🤖 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 83 - 103, In the AliasEngine constructor, don't keep an invalid options.parent on this.#parent; instead test options.parent first and only assign when it's an instance of AliasEngine, otherwise log the error and set this.#parent = null so downstream checks (e.g. calls to getPatterns, getVersion, getAlias or accessing `#words`) behave like a root engine; reposition the instanceof AliasEngine check to control assignment to this.#parent and then initialize this.#depth, this.#state and this.#words accordingly (root vs child) so invalid parents are nulled rather than retained.
🧹 Nitpick comments (1)
packages/tempo/test/engine/engine.guard.test.ts (1)
1-62: ⚡ Quick winLGTM — test suite covers the primary guard behaviors well.
One coverage gap worth noting: there is no test for a whitespace-only input (e.g.
guard.test(' ')). The current implementation would returntruefor such input (the loop exits cleanly after consuming only spaces), which may not be the intended contract. A test assertingfalsewould either catch this or explicitly document the intended behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/test/engine/engine.guard.test.ts` around lines 1 - 62, Add a test asserting that a string consisting only of whitespace is treated as invalid: call createMasterGuard(...) to produce a guard and add an expectation like expect(guard.test(' ')).toBe(false) (or add a new it block alongside the existing "should fail on empty or non-string input" case) to ensure guard.test handles whitespace-only input; locate usage of createMasterGuard and guard.test in engine.guard.test.ts to insert the new assertion.
🤖 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 `@doc/main_branch_protection.md`:
- Around line 127-129: The docs currently recommend using "git commit -am" which
omits new/untracked files; update the recovery flow to stage all changes first
by advising to run "git add ." or "git add -A" before the commit step (replace
or augment the "git commit -am" guidance), and mention the rationale briefly so
users know new files will be included; reference the git commit -am step and the
new recommended git add commands so readers can locate and apply the change.
- Around line 139-143: Update the README step before the destructive command
"git reset --hard origin/main" to add an explicit safety check: instruct users
to verify their working tree is clean (for example by running a status/porcelain
check or confirming there are no uncommitted changes) and to stash or commit any
work before proceeding; include a short warning sentence explaining that the
reset will discard local changes and recommend alternatives (stash/commit) if
the check fails so users don't lose work.
In `@packages/tempo/doc/tempo.layout.md`:
- Around line 88-94: The illustrative console output currently shows sequential
duplicate named groups for the `{sep}` token which is invalid; update the
example output produced by Tempo.regexp('{dd}{sep}{mm}{sep}{yy}') to reflect
Tempo's actual behavior (e.g., deduplicating or renaming groups so names are
unique within the same branch such as `(?<sep>...)(?<sep1>...)` or showing how
Tempo merges repeated tokens) and ensure the comment no longer displays
`(?<sep>...)(?<sep>...)`; reference the Tempo.regexp usage and the format string
'{dd}{sep}{mm}{sep}{yy}' when updating the illustrative output.
In `@packages/tempo/index.md`:
- Around line 558-565: The .tempo-btn-brand and .tempo-btn-brand:hover rules
hardcode light blue colors that fail WCAG contrast; replace them with either (a)
a darker background color that yields at least 4.5:1 contrast against white
(update the values in .tempo-btn-brand and .tempo-btn-brand:hover) or (b) revert
to using the existing theme variables like .tempo-btn-alt does so styling is
consistent and accessible; ensure the new colors are tested to meet 4.5:1 and
keep .tempo-btn-alt’s variable-based approach as the reference for consistency.
In `@packages/tempo/src/engine/engine.guard.ts`:
- Around line 30-31: The test(input: string): boolean in engine.guard.ts
currently returns true for inputs that are only whitespace because it only
checks !input and then advances over spaces; fix by adding a boolean flag (e.g.,
sawNonWhitespace or matchedToken) inside test to track whether any
non-whitespace/token character was actually consumed/matched before returning
true, set it when any token-accepting branch runs (not when skipping spaces),
and use that flag in the final return so pure-whitespace inputs return false;
apply the same change to the other test occurrence referenced (the second test
method around the other guard).
- Around line 18-21: The map step building wordsList can crash when a Symbol has
no description because (isSymbol(w) ? w.description : ... ) may be undefined
before .toLowerCase() — update the logic in the wordsList creation (the chain
around isSymbol, isString, and .map) to guard against undefined descriptions:
either filter out symbols with no description before mapping or coerce undefined
to an empty string/default (e.g., skip or return ''), and then call
.toLowerCase(); specifically change the sequence around wordsList (the
filter(...).map(...) chain) so that any Symbol without a description is excluded
or handled before calling .toLowerCase() to avoid the TypeError.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 829-833: The current use of the nullish-assignment on
state.patternCompiler may reuse a PatternCompiler instance from a
prototype/parent state; change the logic in the accessor where you obtain state
via (this as any)[$Internal]() so that you either always construct a new
PatternCompiler bound to this state or recreate it when an existing
state.patternCompiler is present but its bound state !== state; ensure the
stored field (state.patternCompiler) is set to new PatternCompiler({ state }) in
that case and then call state.patternCompiler.compileRegExp(layout, snippet) so
PatternCompiler ownership and cache remain isolated per-state.
---
Outside diff comments:
In `@packages/tempo/src/engine/engine.alias.ts`:
- Around line 83-103: In the AliasEngine constructor, don't keep an invalid
options.parent on this.#parent; instead test options.parent first and only
assign when it's an instance of AliasEngine, otherwise log the error and set
this.#parent = null so downstream checks (e.g. calls to getPatterns, getVersion,
getAlias or accessing `#words`) behave like a root engine; reposition the
instanceof AliasEngine check to control assignment to this.#parent and then
initialize this.#depth, this.#state and this.#words accordingly (root vs child)
so invalid parents are nulled rather than retained.
---
Nitpick comments:
In `@packages/tempo/test/engine/engine.guard.test.ts`:
- Around line 1-62: Add a test asserting that a string consisting only of
whitespace is treated as invalid: call createMasterGuard(...) to produce a guard
and add an expectation like expect(guard.test(' ')).toBe(false) (or add a new
it block alongside the existing "should fail on empty or non-string input" case)
to ensure guard.test handles whitespace-only input; locate usage of
createMasterGuard and guard.test in engine.guard.test.ts to insert the new
assertion.
🪄 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: 3508f842-c96f-4325-aa34-907a59a46647
📒 Files selected for processing (40)
.agent/workflows/interactive-testing.md.github/workflows/ci.ymldoc/main_branch_protection.mdpackage.jsonpackages/library/package.jsonpackages/tempo/CHANGELOG.mdpackages/tempo/doc/architecture.mdpackages/tempo/doc/installation.mdpackages/tempo/doc/tempo.layout.mdpackages/tempo/index.mdpackages/tempo/package.jsonpackages/tempo/plan/RELEASE-D.mdpackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/engine/engine.alias.tspackages/tempo/src/engine/engine.guard.tspackages/tempo/src/engine/engine.layout.tspackages/tempo/src/engine/engine.pattern.tspackages/tempo/src/engine/engine.resolver.tspackages/tempo/src/parse/parse.layout.tspackages/tempo/src/parse/parse.resolver.tspackages/tempo/src/plugin/plugin.util.tspackages/tempo/src/plugin/term/term.quarter.tspackages/tempo/src/plugin/term/term.season.tspackages/tempo/src/support/support.default.tspackages/tempo/src/support/support.enum.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/support.init.tspackages/tempo/src/support/support.intl.tspackages/tempo/src/support/support.register.tspackages/tempo/src/support/support.runtime.tspackages/tempo/src/support/support.symbol.tspackages/tempo/src/support/support.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tsconfig.jsonpackages/tempo/test/engine/engine.guard.test.tspackages/tempo/test/engine/parse.layout.test.tspackages/tempo/test/engine/pattern_compiler_optimization.test.tspackages/tempo/test/plugins/plugin_registration.test.tspackages/tempo/vitest.config.ts
💤 Files with no reviewable changes (5)
- packages/tempo/src/parse/parse.layout.ts
- packages/tempo/src/tsconfig.json
- packages/tempo/src/parse/parse.resolver.ts
- packages/tempo/vitest.config.ts
- .github/workflows/ci.yml
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
packages/tempo/src/tempo.class.ts (1)
1287-1288: 💤 Low valueMinor:
tzandcalgetters duplicate work.Each getter independently calls
this.toDateTime()andgetTemporalIds(...). When both are read in succession (a common pattern, e.g. intoNowor duration paths), this resolves the ZonedDateTime twice and re-extracts ids twice. If the call sites are hot, consider memoizing per-instance or exposing a single#ids()helper. Low priority —toDateTime()is already memoized via#resolve, so the cost is just the ID extraction.🤖 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 1287 - 1288, tz and cal both call this.toDateTime() and getTemporalIds(...) causing duplicated work; add a single private helper (e.g., a method named `#ids`() or a lazy memoized pair like this.#tz and this.#cal) that calls getTemporalIds(this.toDateTime()) once and returns the tuple, then change the getters tz and cal to return the first and second elements from that helper; ensure the helper is private to the Tempo class and lazily computes/stores the ids so subsequent tz/cal accesses reuse the result.packages/tempo/src/engine/engine.composer.ts (1)
24-24: 💤 Low valueDefensive
dateTime ?? todayfallback is now unreachable — consider tightening typing/intent.With
let dateTime: Temporal.ZonedDateTime = today;on Line 24 and everydefaultbranch returning early (Lines 152, and Line 46 for the string failure path),dateTimeis always assigned by the time control reaches Line 156, sodateTime ?? todayis dead. Either drop the??(since the type is now non-optional) or remove the explicittodayinitializer to keep the fallback meaningful — the current pairing is contradictory.Suggested cleanup
- return { dateTime: dateTime ?? today, timeZone }; + return { dateTime, timeZone };Also applies to: 156-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.composer.ts` at line 24, The variable dateTime is declared as a non-optional ZonedDateTime and initialized to today, making the later defensive "dateTime ?? today" check unreachable; either remove the initializer (let dateTime: Temporal.ZonedDateTime | undefined) so the nullish fallback remains meaningful, or keep the non-optional type and initializer and remove the "?? today" at the use site. Locate the declaration of dateTime (let dateTime: Temporal.ZonedDateTime = today;) and the later usage with "dateTime ?? today" and change one of them so the type and fallback behavior are consistent (adjust the type to allow undefined or drop the nullish operator).packages/tempo/plan/RELEASE-D.md (1)
129-173: 💤 Low valueDetailed Outlines for Guard Builder/Normalizer still read as "Assessment Outline" despite checklist showing completed extraction.
The checklist (Lines 24-35) marks both Guard Builder Extraction and Parse Result Normalizer Extraction as completed (with concrete extractions to
engine.guard.tsandengine.normalizer.ts), but the corresponding detailed outlines below (Lines 129-173) are still phrased as evaluation/assessment of feasibility ("Evaluate the value and feasibility...", "Assessment Steps", "If justified, outline module boundaries..."). Worth updating these sections to reflect the realized design (modules, exported APIs likecreateMasterGuard,normalizeMatch,accumulateResult) so the plan stays consistent with the implemented state.🤖 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/plan/RELEASE-D.md` around lines 129 - 173, Update the "Guard Builder Extraction" and "Parse Result Normalizer Extraction" sections to reflect that extraction is complete: replace assessment language with concrete design and migration details, mention the new modules (engine.guard.ts and engine.normalizer.ts) and their exported APIs such as createMasterGuard, normalizeMatch, and accumulateResult, describe how they integrate with tempo.class.ts's parse pipeline and pattern system, and remove/replace "Assessment Steps" with brief notes on module responsibilities, public function signatures, and any migration/usage examples so the document matches the implemented state.packages/tempo/src/engine/engine.normalizer.ts (3)
88-178: 💤 Low value
prevAnchoris shadowed inside the alias loop — high risk of future regressions.Line 88 captures the caller's anchor; line 91 overwrites
state.anchorto the currentdateTime. Inside the alias-resolution branch, line 158 re-declaresconst prevAnchor: any = state.anchor;to snapshot the current anchor beforesubParse, then line 161 restores it. The outerfinallyat line 172 then restores the original outerprevAnchor. Functionally correct today because they are in separate scopes, but the identical name across nested scopes is a maintenance hazard — a stray edit promoting either declaration (or fusing the blocks) silently destroys the save/restore protocol.Rename the inner one (e.g.
innerPrevAnchororsubAnchor) to make the layered restore explicit.♻️ Disambiguate
- const prevAnchor: any = state.anchor; - state.anchor = dateTime; - const resMatch = subParse(res.value, dateTime, resolving); - state.anchor = prevAnchor; + const subPrevAnchor: any = state.anchor; + state.anchor = dateTime; + const resMatch = subParse(res.value, dateTime, resolving); + state.anchor = subPrevAnchor;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.normalizer.ts` around lines 88 - 178, The inner snapshot of state.anchor inside the alias-resolution branch shadows the outer prevAnchor; rename the inner const prevAnchor (the one declared before calling subParse) to a distinct identifier like innerPrevAnchor or subAnchor and update its corresponding restore (state.anchor = innerPrevAnchor) so the outer prevAnchor used in the outer finally block (and the outer save/restore logic around state.anchor/state.zdt) remains untouched; locate the declaration and restore around subParse in the alias handling code path (the block that sets state.anchor = dateTime, calls subParse, then restores state.anchor) and rename the local variable consistently.
127-133: 💤 Low valueMagic recursion limit
> 50.The hard-coded
50mirrors no other constant in this file and is unrelated to the depth limit (> 10) used insidePatternCompiler.matcher. Hoist it to a named constant and document why this ceiling was chosen, so future tuning (or test injection) doesn't have to grep for50.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.normalizer.ts` around lines 127 - 133, Replace the magic number 50 used in the recursion guard inside the normalization loop with a well-named constant (e.g., MAX_TEMPO_RESOLVE_DEPTH) declared at the top of the module and add a short comment explaining why this ceiling was chosen and how it relates to the PatternCompiler.matcher depth limit (~10); update the check from "resolvingKeys.size > 50" to use that constant, leaving the rest of the logic (setting state.errored, calling (TempoClass as any)[sym.$logError], deleting groups[key], and continue) unchanged so behavior is identical but the limit is discoverable and configurable.
40-41: 💤 Low valueRedundant
getTemporalIdsevaluations intz/calgetters.Each property access recomputes the full normalization.
caleven passesstate.config.timeZoneonly to discard[0]. Trivial to consolidate withtoNowabove.♻️ Compute once
- toNow: () => { - const [tz, cal] = getTemporalIds(state.config.timeZone, state.config.calendar); - return instant().toZonedDateTimeISO(tz).withCalendar(cal); - }, - get tz() { return getTemporalIds(state.config.timeZone)[0] }, - get cal() { return getTemporalIds(state.config.timeZone, state.config.calendar)[1] }, + toNow: () => { + const [tz, cal] = getTemporalIds(state.config.timeZone, state.config.calendar); + return instant().toZonedDateTimeISO(tz).withCalendar(cal); + }, + get tz() { return getTemporalIds(state.config.timeZone, state.config.calendar)[0] }, + get cal() { return getTemporalIds(state.config.timeZone, state.config.calendar)[1] },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.normalizer.ts` around lines 40 - 41, The tz and cal getters call getTemporalIds separately (and cal calls it with only timeZone), causing duplicate work; compute getTemporalIds once (e.g., call getTemporalIds(state.config.timeZone, state.config.calendar) once above or alongside toNow and store the tuple/values) and have the tz getter return the first element and cal return the second, removing the redundant per-getter calls and the incorrect single-argument call in cal.packages/library/src/common/temporal.library.ts (1)
86-89: ⚡ Quick win
toZonedDateTimehas a design flaw with string inputs that should be fixed to prevent future issues.The function accepts ISO strings in its signature but blindly appends
[${tz}]without checking if the input already contains a zone designator. Passing'2024-01-01T00:00:00-05:00[America/New_York]'would create'2024-01-01T00:00:00-05:00[America/New_York][UTC]'(invalid per Temporal spec), and'2024-01-01T00:00Z'with a differenttzparameter would produce'2024-01-01T00:00Z[UTC]'(which silently succeeds only if thetzmatches Z).Current code avoids this by passing property bags only (not strings), but since this is a reachable public API, adding a guard is sensible to prevent misuse:
Suggested fix
export function toZonedDateTime(bag: Temporal.ZonedDateTimeLike | string, tz: Temporal.TimeZoneLike = 'UTC'): Temporal.ZonedDateTime { - if (isString(bag)) return Temporal.ZonedDateTime.from(`${bag}[${tz}]`); + if (isString(bag)) { + const hasZoneBracket = /\[[^\]]+\]\s*$/.test(bag); + return Temporal.ZonedDateTime.from(hasZoneBracket ? bag : `${bag}[${String(tz)}]`); + } return Temporal.ZonedDateTime.from(bag); }🤖 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/library/src/common/temporal.library.ts` around lines 86 - 89, The toZonedDateTime function currently appends `[${tz}]` to any string input which can produce invalid double-zone strings; update toZonedDateTime to first detect whether the string already includes a zone designator (either an offset like ±HH:MM or a bracketed IANA zone like [...]) and only append `[${tz}]` when no zone is present, otherwise pass the original string through to Temporal.ZonedDateTime.from; keep the existing behavior for non-string bags. Use a simple regex test on the string input inside toZonedDateTime to decide which branch to call Temporal.ZonedDateTime.from on.packages/tempo/src/engine/engine.pattern.ts (1)
79-83: 💤 Low valueAdd defensive placeholder name sanitization to prevent invalid named-capture identifiers.
The current code at lines 79-83 only escapes dots (
name.replace(/\./g, '_')), but JS named capture groups require identifiers matching[A-Za-z_$][A-Za-z0-9_$]*. If a placeholder name starts with a digit (e.g.,{123ext}) or contains other special characters,new RegExp()will throw at line 93—but the error is silently caught and falls back to escaped literal matching, masking the real layout/snippet authoring bug.🛡️ Defensive sanitization
- if (res && name.includes('.')) { // wrap dotted extensions for identification - const safeName = name.replace(/\./g, '_'); - if (!res.startsWith(`(?<${safeName}>`)) - res = `(?<${safeName}>${res})`; - } + if (res && name.includes('.')) { // wrap dotted extensions for identification + const safeName = name.replace(/[^A-Za-z0-9_$]/g, '_'); + if (/^[A-Za-z_$]/.test(safeName) && !res.startsWith(`(?<${safeName}>`)) + res = `(?<${safeName}>${res})`; + }🤖 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.pattern.ts` around lines 79 - 83, The placeholder sanitization only replaces dots but must produce a valid JS named-capture identifier; update the logic that builds safeName (the code that currently does const safeName = name.replace(/\./g, '_')) to: trim/ensure non-empty, replace any character not in [A-Za-z0-9_$] with '_', and if the first character is not [A-Za-z_$] prefix an underscore (or replace it) so the final safeName matches [A-Za-z_$][A-Za-z0-9_$]* before it is interpolated into the group `(?<${safeName}>${res})`; keep the existing fallback behavior for RegExp construction (the code that calls new RegExp on res) but ensure we generate a valid safeName to avoid throwing there and to surface real template issues instead of silently falling back.
🤖 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 `@doc/main_branch_protection.md`:
- Around line 98-101: Remove or relocate the premature "Stage all changes" and
"Commit your work" steps in the introductory section that currently advise
running `git add .` / `git add -A` and `git commit -m "Your descriptive
message"` before creating a branch; these instructions conflict with the
pre-commit hook described in the scenario and already appear correctly in the
branch-first workflows below—either delete those two steps or move them into the
method sections that instruct creating a new branch first so staging/commit
guidance appears only after branch creation.
In `@packages/library/src/common/temporal.library.ts`:
- Around line 115-139: The guard in getTemporalIds wrongfully treats a
ZonedDateTime as a timeZone-like object when a Calendar is passed as the second
argument; add explicit TypeScript overloads for getTemporalIds to support
(tzOrZdt: ZonedDateTime, cal?: Calendar), (tzOrZdt: TimeZoneLike, cal?:
Calendar) and the general fallback to improve type safety, then change the
runtime guard in getTemporalIds (currently using isZonedDateTime(tzOrZdt) &&
isUndefined(cal)) to detect ZonedDateTime regardless of whether a calendar arg
was supplied (e.g., if isZonedDateTime(tzOrZdt) then use its
timeZoneId/calendarId and, if cal is provided, treat that as the explicit
calendar override), and ensure engine.normalizer.ts callers (tz and cal getters)
rely on the typed overloads so swapped-argument mistakes are caught by the
compiler.
In `@packages/tempo/src/engine/engine.normalizer.ts`:
- Around line 192-205: accumulateResult currently builds a fresh match object
each call so the res.includes(match) check is a no-op; either remove that check
to always append, or implement real deduping by comparing stable identity
fields. Update the function accumulateResult to compute a stable identity (e.g.
combine match.match and match.source or compare match.match, match.source,
match.anchor as appropriate) and replace res.includes(match) with a lookup like
res.some(existing => existing.match === match.match && existing.source ===
match.source [&& existing.anchor === match.anchor if needed]) before pushing, or
simply delete the includes branch if deduplication is not required; refer to
symbols: accumulateResult, match, state.parse.result, and state.parse.anchor.
---
Nitpick comments:
In `@packages/library/src/common/temporal.library.ts`:
- Around line 86-89: The toZonedDateTime function currently appends `[${tz}]` to
any string input which can produce invalid double-zone strings; update
toZonedDateTime to first detect whether the string already includes a zone
designator (either an offset like ±HH:MM or a bracketed IANA zone like [...])
and only append `[${tz}]` when no zone is present, otherwise pass the original
string through to Temporal.ZonedDateTime.from; keep the existing behavior for
non-string bags. Use a simple regex test on the string input inside
toZonedDateTime to decide which branch to call Temporal.ZonedDateTime.from on.
In `@packages/tempo/plan/RELEASE-D.md`:
- Around line 129-173: Update the "Guard Builder Extraction" and "Parse Result
Normalizer Extraction" sections to reflect that extraction is complete: replace
assessment language with concrete design and migration details, mention the new
modules (engine.guard.ts and engine.normalizer.ts) and their exported APIs such
as createMasterGuard, normalizeMatch, and accumulateResult, describe how they
integrate with tempo.class.ts's parse pipeline and pattern system, and
remove/replace "Assessment Steps" with brief notes on module responsibilities,
public function signatures, and any migration/usage examples so the document
matches the implemented state.
In `@packages/tempo/src/engine/engine.composer.ts`:
- Line 24: The variable dateTime is declared as a non-optional ZonedDateTime and
initialized to today, making the later defensive "dateTime ?? today" check
unreachable; either remove the initializer (let dateTime: Temporal.ZonedDateTime
| undefined) so the nullish fallback remains meaningful, or keep the
non-optional type and initializer and remove the "?? today" at the use site.
Locate the declaration of dateTime (let dateTime: Temporal.ZonedDateTime =
today;) and the later usage with "dateTime ?? today" and change one of them so
the type and fallback behavior are consistent (adjust the type to allow
undefined or drop the nullish operator).
In `@packages/tempo/src/engine/engine.normalizer.ts`:
- Around line 88-178: The inner snapshot of state.anchor inside the
alias-resolution branch shadows the outer prevAnchor; rename the inner const
prevAnchor (the one declared before calling subParse) to a distinct identifier
like innerPrevAnchor or subAnchor and update its corresponding restore
(state.anchor = innerPrevAnchor) so the outer prevAnchor used in the outer
finally block (and the outer save/restore logic around state.anchor/state.zdt)
remains untouched; locate the declaration and restore around subParse in the
alias handling code path (the block that sets state.anchor = dateTime, calls
subParse, then restores state.anchor) and rename the local variable
consistently.
- Around line 127-133: Replace the magic number 50 used in the recursion guard
inside the normalization loop with a well-named constant (e.g.,
MAX_TEMPO_RESOLVE_DEPTH) declared at the top of the module and add a short
comment explaining why this ceiling was chosen and how it relates to the
PatternCompiler.matcher depth limit (~10); update the check from
"resolvingKeys.size > 50" to use that constant, leaving the rest of the logic
(setting state.errored, calling (TempoClass as any)[sym.$logError], deleting
groups[key], and continue) unchanged so behavior is identical but the limit is
discoverable and configurable.
- Around line 40-41: The tz and cal getters call getTemporalIds separately (and
cal calls it with only timeZone), causing duplicate work; compute getTemporalIds
once (e.g., call getTemporalIds(state.config.timeZone, state.config.calendar)
once above or alongside toNow and store the tuple/values) and have the tz getter
return the first element and cal return the second, removing the redundant
per-getter calls and the incorrect single-argument call in cal.
In `@packages/tempo/src/engine/engine.pattern.ts`:
- Around line 79-83: The placeholder sanitization only replaces dots but must
produce a valid JS named-capture identifier; update the logic that builds
safeName (the code that currently does const safeName = name.replace(/\./g,
'_')) to: trim/ensure non-empty, replace any character not in [A-Za-z0-9_$] with
'_', and if the first character is not [A-Za-z_$] prefix an underscore (or
replace it) so the final safeName matches [A-Za-z_$][A-Za-z0-9_$]* before it is
interpolated into the group `(?<${safeName}>${res})`; keep the existing fallback
behavior for RegExp construction (the code that calls new RegExp on res) but
ensure we generate a valid safeName to avoid throwing there and to surface real
template issues instead of silently falling back.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 1287-1288: tz and cal both call this.toDateTime() and
getTemporalIds(...) causing duplicated work; add a single private helper (e.g.,
a method named `#ids`() or a lazy memoized pair like this.#tz and this.#cal) that
calls getTemporalIds(this.toDateTime()) once and returns the tuple, then change
the getters tz and cal to return the first and second elements from that helper;
ensure the helper is private to the Tempo class and lazily computes/stores the
ids so subsequent tz/cal accesses reuse the result.
🪄 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: 2ee03d44-ceda-475e-9e94-2bfc6af60d77
📒 Files selected for processing (19)
doc/main_branch_protection.mdpackages/library/src/common/assertion.library.tspackages/library/src/common/temporal.library.tspackages/tempo/doc/tempo.layout.mdpackages/tempo/index.mdpackages/tempo/plan/RELEASE-D.mdpackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/engine/engine.alias.tspackages/tempo/src/engine/engine.composer.tspackages/tempo/src/engine/engine.guard.tspackages/tempo/src/engine/engine.normalizer.tspackages/tempo/src/engine/engine.pattern.tspackages/tempo/src/engine/engine.term.tspackages/tempo/src/module/module.duration.tspackages/tempo/src/support/support.default.tspackages/tempo/src/tempo.class.tspackages/tempo/test/core/tempo_guard.test.tspackages/tempo/test/engine/engine.guard.test.tspackages/tempo/test/support/setup.console-spy.ts
✅ Files skipped from review due to trivial changes (4)
- packages/tempo/src/engine/engine.term.ts
- packages/tempo/test/support/setup.console-spy.ts
- packages/tempo/src/engine/engine.guard.ts
- packages/tempo/src/support/support.default.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/tempo/index.md
- packages/tempo/test/engine/engine.guard.test.ts
- packages/tempo/src/engine/engine.alias.ts
Owner
Author
|
@coderabbitai full review please |
|
Tip For best results, initiate chat on the files or code changes.
[full_review] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Guard Builder isolation
Summary by CodeRabbit