submit for GH review#43
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 8 minutes and 30 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughTempo v3.1.0 refactors configuration around a nested ChangesTempo v3.1.0 Registry Refactoring and Format Modifiers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/tempo/test/discrete/format.test.ts (1)
87-89: ⚡ Quick winAdd scoped-term localization coverage (
{#timeOfDay}) in this suite.Current assertions cover
{#tod}but not the scoped accessor form. Adding one expectation here will guard the object-backed term localization path from regressions.Suggested test addition
it('should automatically localize Terms', () => { expect(t.format('{`#tod`}')).toBe('Milieu de la matinée'); + expect(t.format('{`#timeOfDay`}')).toBe('Milieu de la matinée'); })🤖 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/discrete/format.test.ts` around lines 87 - 89, Add an assertion in the same test ("should automatically localize Terms") to exercise the scoped-term accessor form by calling t.format with the scoped key string '{`#timeOfDay`}' and expecting the same localized string as '{`#tod`}' (e.g., 'Milieu de la matinée'); this ensures the object-backed term lookup path (the t.format function and the scoped accessor '{`#timeOfDay`}') is covered alongside the existing '{`#tod`}' case.
🤖 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/doc/tempo.cookbook.md`:
- Around line 242-245: The example sets localize: true at the top level of
Tempo.config but runtime reads config.format.localize, so update the example to
set the localize flag under the format object when calling Tempo.config (i.e.,
use Tempo.config({ locale: 'fr-FR', format: { localize: true } })) and ensure
this matches how the code checks config.format.localize in the Tempo
implementation.
In `@packages/tempo/src/module/module.format.ts`:
- Around line 167-187: The localization lookup currently forces termVal to a
string via String(termVal) which breaks object-backed term values (e.g., scoped
accessors returning objects) causing lookups for "[object Object]"; update the
logic around valStr in the block handling token.startsWith('#') (where termKey,
termName, termVal, plugin are used) to first detect object-backed termVal and
derive a proper lookup key (e.g., prefer termVal.key, termVal.id, or a
designated lookup property) or otherwise serialize deterministically, and only
fall back to String(termVal) for primitives—then use that derived key for the
registry and plugin group locale lookups (flatGroups/group -> locale) so
object-backed terms resolve correctly.
In `@packages/tempo/src/support/support.init.ts`:
- Around line 40-43: The nested-shallow copies for state.config.registry and
state.config.format are being lost later when state.config gets replaced again
through the local baseState path, allowing writes to leak into
baseState.config.registry; fix by ensuring every time state.config is cloned
from baseState.config (including the later replacement path) you also create
fresh nested shadow objects for registry and format (e.g., using
Object.create(baseState.config.registry) /
Object.create(baseState.config.format) or an equivalent shallow clone) and
similarly preserve state.parse cloning (state.parse =
Object.create(baseState.parse)) so that no nested references to baseState stay
shared.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 764-768: The current registry default uses config.registry ??
{...} which lets a partial registry object override both fields; change to
normalize each registry field individually so formats and locales are resolved
per-field (e.g., use config.registry?.formats ?? config.formats ??
enumify(STATE.FORMAT, false) and config.registry?.locales ?? config.locales ??
proxify(STATE.LOCALE, true, true)) so accesses like
this.#local.config.registry.formats[key] never encounter an undefined formats
object; keep the existing format default (config.format ?? { localize:
config.localize ?? false }) but ensure registry.formats and registry.locales are
set independently.
---
Nitpick comments:
In `@packages/tempo/test/discrete/format.test.ts`:
- Around line 87-89: Add an assertion in the same test ("should automatically
localize Terms") to exercise the scoped-term accessor form by calling t.format
with the scoped key string '{`#timeOfDay`}' and expecting the same localized
string as '{`#tod`}' (e.g., 'Milieu de la matinée'); this ensures the
object-backed term lookup path (the t.format function and the scoped accessor
'{`#timeOfDay`}') is covered alongside the existing '{`#tod`}' case.
🪄 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: 791ca747-ecfa-4ba4-8b12-0868c84eccaf
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
package.jsonpackages/library/package.jsonpackages/library/src/common/international.library.tspackages/tempo/.vitepress/config.tspackages/tempo/CHANGELOG.mdpackages/tempo/doc/release-notes-v3.0.0.mdpackages/tempo/doc/releases/index.mdpackages/tempo/doc/releases/v3.x.mdpackages/tempo/doc/releases/v4.x.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/doc/tempo.registry.mdpackages/tempo/package.jsonpackages/tempo/src/module/module.format.tspackages/tempo/src/plugin/term/term.timeline.tspackages/tempo/src/plugin/term/term.type.tspackages/tempo/src/support/support.default.tspackages/tempo/src/support/support.enum.tspackages/tempo/src/support/support.init.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tempo.version.tspackages/tempo/test/discrete/format.test.tspackages/tempo/test/engine/meridiem.test.ts
💤 Files with no reviewable changes (1)
- packages/tempo/doc/release-notes-v3.0.0.md
Contributor
There was a problem hiding this comment.
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.cookbook.md (1)
255-256:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncorrect nesting for
localesconfiguration; must nest underregistry.The
localesoption must be nested insideregistryto match the Tempo type contract. The code currently showslocalesat the top level of the config object, but the runtime readsconfig?.registry?.locales, causing user-provided locale overrides to be silently ignored.🐛 Proposed fix
Tempo.config({ - locales: { + registry: { + locales: { fr: { morning: 'Matinée', afternoon: 'Après-midi', ordinal: (n) => n === 1 ? '1er' : `${n}e` } + } } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/doc/tempo.cookbook.md` around lines 255 - 256, The config example is nesting locales at the top level so user locale overrides are ignored because the runtime reads config?.registry?.locales; update the Tempo.config example so that the locales object is placed under a registry key (i.e., Tempo.config({ registry: { locales: { ... } } })) so the config matches the expected shape used by the code that accesses config?.registry?.locales.
🧹 Nitpick comments (1)
packages/tempo/doc/tempo.cookbook.md (1)
242-242: 💤 Low valueSimplify wording: "prior to" → "before".
The phrase "prior to casing modifiers" is slightly wordy; "before casing modifiers" is clearer and more concise.
📝 Proposed fix
-To avoid repeatedly typing `:locale` on every token, you can set `format: { localize: true }` in your global `Tempo.config`. This will automatically append the `:locale` modifier (prior to casing modifiers) for all format evaluations: +To avoid repeatedly typing `:locale` on every token, you can set `format: { localize: true }` in your global `Tempo.config`. This will automatically append the `:locale` modifier (before casing modifiers) for all format evaluations:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/doc/tempo.cookbook.md` at line 242, Summary: replace wordy phrase "prior to casing modifiers" with the clearer "before casing modifiers". Locate the sentence that reads "This will automatically append the `:locale` modifier (prior to casing modifiers) for all format evaluations" and edit it to "This will automatically append the `:locale` modifier (before casing modifiers) for all format evaluations" so the wording is more concise and consistent.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@packages/tempo/doc/tempo.cookbook.md`:
- Around line 255-256: The config example is nesting locales at the top level so
user locale overrides are ignored because the runtime reads
config?.registry?.locales; update the Tempo.config example so that the locales
object is placed under a registry key (i.e., Tempo.config({ registry: { locales:
{ ... } } })) so the config matches the expected shape used by the code that
accesses config?.registry?.locales.
---
Nitpick comments:
In `@packages/tempo/doc/tempo.cookbook.md`:
- Line 242: Summary: replace wordy phrase "prior to casing modifiers" with the
clearer "before casing modifiers". Locate the sentence that reads "This will
automatically append the `:locale` modifier (prior to casing modifiers) for all
format evaluations" and edit it to "This will automatically append the `:locale`
modifier (before casing modifiers) for all format evaluations" so the wording is
more concise and consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dfc5e202-810b-4aa9-abc9-d82254e3259d
📒 Files selected for processing (8)
packages/library/src/common/international.library.tspackages/library/src/common/string.library.tspackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/src/module/module.format.tspackages/tempo/src/support/support.init.tspackages/tempo/src/tempo.class.tspackages/tempo/test/discrete/format.test.ts
💤 Files with no reviewable changes (1)
- packages/tempo/doc/tempo.config.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/library/src/common/international.library.ts
- packages/tempo/test/discrete/format.test.ts
- packages/tempo/src/tempo.class.ts
- packages/tempo/src/support/support.init.ts
- packages/tempo/src/module/module.format.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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/support/support.init.ts (1)
216-220:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLocale/localize toggles only add localized rules; they never unwind them.
The rebuild path only runs for
localize && lang !== 'en', and disabling localization never marks the parser dirty. Once a state has loaded localized snippets/events, switching tolocalize: falseor back to an English locale leaves the oldlocaleMapand aliases in place, so later parses can keep accepting the previous locale.Also applies to: 231-235, 338-366
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/support/support.init.ts` around lines 216 - 220, The locale handling only adds localized rules and never removes them; update the locale change handling (the 'locale' case using canonicalLocale and setProperty on state.config) to detect when localization is being removed or when the locale changes back to an English/default locale and then clear any existing locale-specific state (e.g., localeMap, locale aliases) and mark the parser/state as dirty so rebuilds run; specifically, after setting the new locale in setProperty, call the existing rebuild/mark-dirty routine (or add one) for the parser and explicitly clear or reset any localeMap/alias collections so disabling localize or switching to 'en' removes previously applied localized rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tempo/CHANGELOG.md`:
- Line 27: Update the fallback description text in both places to match actual
behavior: replace the current "Global Registry > Plugin Bundled Dictionary >
Plugin Default Label" wording with a clear registry/bundled-dictionary flow that
falls back to the term's existing label/value when neither source resolves
(e.g., "Global Registry > Plugin Bundled Dictionary > term's existing
label/value"); apply this exact rewording at packages/tempo/CHANGELOG.md lines
27-27 and mirror the same wording at packages/tempo/doc/releases/v3.x.md lines
12-12.
In `@packages/tempo/src/engine/engine.alias.ts`:
- Around line 85-89: The constructor currently checks isObject(parent) &&
'registerAliases' in parent then uses (parent as any).#state and .#words which
can throw for non-AliasEngine objects; change the runtime guard to use parent
instanceof AliasEngine (or otherwise validate via a public API) so you only
inherit private state/words from a true AliasEngine; update the branch that
assigns this.#parent, this.#depth, this.#state, and this.#words to cast to
AliasEngine after the new instanceof check and remove the duck-typed
"'registerAliases' in parent" condition.
In `@packages/tempo/src/module/module.parse.ts`:
- Around line 68-74: The branch in module.parse.ts that handles "else if
(isDefined(val))" drops the caller-provided anchor when TempoClass is not
registered; change the fallback so a defined val is preserved instead of
unconditionally using instant(): when TempoClass is present keep the existing
(TempoClass as any).from(val, safeConfig).toDateTime(), and when TempoClass is
absent convert the supplied val into a zoned DateTime using a simple JS->zoned
conversion (e.g., treat val as ISO/number/Date and create a ZonedDateTime in tz
with calendar cal) and assign that to basis; keep the safeConfig handling
(delete safeConfig.anchor) intact. Ensure the code still falls back to instant()
only when val is undefined.
In `@packages/tempo/src/support/support.init.ts`:
- Around line 227-235: The parse option mutations only update state.parse and
leave state.config.parse stale; update both places: in the 'parse' case (where
Object.assign(state.parse, arg.value) runs) ensure state.config.parse exists
(create with setProperty or {}) and merge the same values into
state.config.parse as well; in the 'localize' case, also set
state.config.parse.localize = Boolean(arg.value) in addition to
state.parse.localize so config.parse stays synchronized with state.parse.
In `@packages/tempo/test/discrete/parse.locale.test.ts`:
- Around line 5-11: The test uses Tempo.init(...) in beforeEach but never resets
the Tempo singleton, so prior tests can leak state; modify the setup to clear
the singleton before reinitializing by either calling the provided reset API
(e.g., Tempo.reset() or Tempo.resetRuntime() if available) or explicitly
clearing the internal runtime (e.g., delete Tempo.runtime or set Tempo.runtime =
undefined) and then call Tempo.init({ mode: 'strict', timeZone: 'UTC', calendar:
'iso8601' }) so each test gets a fresh baseline; update the beforeEach in the
test to perform that reset step before calling Tempo.init.
---
Outside diff comments:
In `@packages/tempo/src/support/support.init.ts`:
- Around line 216-220: The locale handling only adds localized rules and never
removes them; update the locale change handling (the 'locale' case using
canonicalLocale and setProperty on state.config) to detect when localization is
being removed or when the locale changes back to an English/default locale and
then clear any existing locale-specific state (e.g., localeMap, locale aliases)
and mark the parser/state as dirty so rebuilds run; specifically, after setting
the new locale in setProperty, call the existing rebuild/mark-dirty routine (or
add one) for the parser and explicitly clear or reset any localeMap/alias
collections so disabling localize or switching to 'en' removes previously
applied localized rules.
🪄 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: 8bf60d7d-bfe5-4735-930d-f7b9c235b640
📒 Files selected for processing (12)
packages/tempo/CHANGELOG.mdpackages/tempo/doc/releases/v3.x.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/src/engine/engine.alias.tspackages/tempo/src/engine/engine.normalizer.tspackages/tempo/src/module/module.parse.tspackages/tempo/src/support/support.init.tspackages/tempo/src/support/support.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/test/discrete/parse.locale.test.tspackages/tempo/test/engine/month-day.test.ts
💤 Files with no reviewable changes (1)
- packages/tempo/test/engine/month-day.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/tempo/doc/tempo.cookbook.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tempo/src/tempo.class.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/doc/tempo.cookbook.md`:
- Around line 102-105: Update the tip that currently says setting `locale`
enables translated parsing to explicitly state that localized parsing also
requires enabling the parser option `parse: { localize: true }` (or the
top-level `localize: true`) at runtime; change the text in the tip to mention
both the `locale` configuration and the required `parse.localize`/top-level
`localize` flag so users know they must enable localization in parsing as well
as set the locale.
In `@packages/tempo/doc/tempo.parse.md`:
- Around line 135-149: Example "new Tempo('vendredi prochain')" conflicts with
the documented limitation that affix/connectors (like "next") are English-biased
when using parse: { localize: true }; update the example and text to avoid
implying localized connector parsing works automatically by either replacing
that example with a purely localized token that Intl provides (e.g., "new
Tempo('vendredi')" or "new Tempo('15 fevrier 2026')") or by explicitly noting
that connector words must be in English unless custom affix mapping is supplied;
reference the Tempo constructor and the parse: { localize: true } option in the
update so readers see where the limitation applies.
🪄 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: 3845f740-70f0-4500-b9df-3d884ea71e0c
📒 Files selected for processing (12)
packages/tempo/CHANGELOG.mdpackages/tempo/doc/releases/v3.x.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/doc/tempo.parse.mdpackages/tempo/src/engine/engine.alias.tspackages/tempo/src/module/module.parse.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/support.init.tspackages/tempo/test/core/alias-engine.test.tspackages/tempo/test/discrete/parse.locale.test.tspackages/tempo/test/discrete/standalone_parse.test.ts
💤 Files with no reviewable changes (1)
- packages/tempo/test/discrete/standalone_parse.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/tempo/src/support/support.index.ts
- packages/tempo/CHANGELOG.md
- packages/tempo/doc/releases/v3.x.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/tempo/test/discrete/parse.locale.test.ts
- packages/tempo/src/support/support.init.ts
- packages/tempo/doc/tempo.config.md
- packages/tempo/src/module/module.parse.ts
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.
Summary by CodeRabbit
New Features
Documentation
Chores