Feature/localize#45
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces Tempo 4.0.0 with breaking registry consolidation and extensive multilingual/Intl formatting improvements; consolidates top-level config keys into unified ChangesConfiguration Registry Consolidation (Breaking Change for 4.0.0)
Intl Formatting API Extension
Multilingual Parsing with Locale-Specific Maps
Ticker Commercialization, License Registry & Branding
sequenceDiagram
participant Caller
participant format as format(fmt?, options?)
participant normalize as normalize args
participant getDTF as getDTF(locale, options)
participant zdt as ZonedDateTime
Caller->>format: format(options: Intl.DateTimeFormatOptions)
format->>normalize: swap fmt↔options if fmt is object
normalize-->>format: (fmt, options)
format->>zdt: apply withTimeZone/withCalendar if config differs
zdt-->>format: adjusted zdt
format->>getDTF: create or retrieve DateTimeFormat
getDTF-->>format: formatter instance
format->>getDTF: .format(zdt)
alt success
getDTF-->>Caller: return formatted string
else RangeError/TypeError
format->>zdt: toLocaleString(locale, safeOptions without timeZone/calendar)
zdt-->>Caller: return fallback string
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
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/src/module/module.format.ts`:
- Around line 91-96: The withTimeZone and withCalendar method calls on lines 92
and 95 can throw RangeError or TypeError when invalid identifiers are provided,
but these calls are outside the try/catch block that handles the Intl fallback
(lines 98-110). Wrap each call to zdt.withTimeZone(config.timeZone) and
zdt.withCalendar(config.calendar) in separate try/catch blocks to catch these
exceptions and allow the code to fall through to the existing fallback behavior
instead of crashing the formatter.
- Around line 37-39: The overload signature at line 37 declares that format()
returns bigint when called with '{nano}', but at runtime the '{nano}' template
is not registered in the FORMAT registry, so it's treated as a literal string
template. The token 'nano' is then resolved to a string value via toString() and
returned as a string at the end of the function, violating the type contract.
Change the return type of the first overload signature from bigint to string to
match the actual runtime behavior when '{nano}' is passed.
🪄 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: e836c8fe-c944-48e3-8abd-a2d978be38d9
⛔ Files ignored due to path filters (2)
packages/tempo/img/tempo-logo.svgis excluded by!**/*.svgpackages/tempo/public/tempo-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (19)
package.jsonpackages/library/package.jsonpackages/tempo/.vitepress/config.tspackages/tempo/README.mdpackages/tempo/doc/architecture.mdpackages/tempo/doc/commercial.mdpackages/tempo/doc/migration-guide.mdpackages/tempo/doc/releases/v3.x.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/doc/tempo.modularity.mdpackages/tempo/doc/tempo.plugin.mdpackages/tempo/doc/tempo.shorthand.mdpackages/tempo/doc/tempo.term.mdpackages/tempo/index.mdpackages/tempo/package.jsonpackages/tempo/src/module/module.format.tspackages/tempo/src/support/support.enum.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.version.ts
💤 Files with no reviewable changes (1)
- packages/tempo/doc/tempo.term.md
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
3-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
version:syncnow conflicts with split package versions (Tempo 3.2.0 vs root/library 3.1.2).Line 3 sets the root to
3.1.2, but Line 21 still syncs both workspaces fromnpm_package_version. Running it will downgrade@magmacomputing/tempoand break the intended release contract (and downstreamTEMPO_VERSIONstamping).Suggested fix
- "version:sync": "node -e \"require('child_process').execSync('npm version ' + process.env.npm_package_version + ' -w `@magmacomputing/tempo` -w `@magmacomputing/library` --no-git-tag-version', {stdio:'inherit'})\"", + "version:sync:library": "npm version 3.1.2 -w `@magmacomputing/library` --no-git-tag-version", + "version:sync:tempo": "npm version 3.2.0 -w `@magmacomputing/tempo` --no-git-tag-version", + "version:sync": "npm run version:sync:library && npm run version:sync:tempo",🤖 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 `@package.json` at line 3, The root package.json version on line 3 is set to 3.1.2, which conflicts with the `@magmacomputing/tempo` workspace that should be at version 3.2.0. The version:sync mechanism uses npm_package_version from the root, causing it to downgrade Tempo when synced. Update the version field on line 3 from 3.1.2 to 3.2.0 to align with the intended Tempo release version and prevent the downgrade conflict with the version:sync process.
🤖 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/support/support.init.ts`:
- Around line 234-239: The clearLocalization() function is only invoked when
resolvedLocales contains a single English locale, but when resolvedLocales is an
all-English array with multiple locales, the code at lines 368-369 skips
regeneration without clearing stale localization data. This leaves stale
monthMap and weekdayMap from previous non-English configurations. Modify the
logic to call clearLocalization() whenever all resolvedLocales are English
(check if resolvedLocales.every(locale => locale.split('-')[0] === 'en')), not
just when the length is 1. Apply this fix at both locations where English
locales are detected: within the conditional at line 238 for the single locale
case, and add the same clearLocalization() call when skipping regeneration for
all-English arrays at lines 368-369.
---
Outside diff comments:
In `@package.json`:
- Line 3: The root package.json version on line 3 is set to 3.1.2, which
conflicts with the `@magmacomputing/tempo` workspace that should be at version
3.2.0. The version:sync mechanism uses npm_package_version from the root,
causing it to downgrade Tempo when synced. Update the version field on line 3
from 3.1.2 to 3.2.0 to align with the intended Tempo release version and prevent
the downgrade conflict with the version:sync process.
🪄 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: 1f114c3d-9284-4b1a-bf4e-d6ae6f9da483
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
package.jsonpackages/tempo/CHANGELOG.mdpackages/tempo/doc/tempo.parse.mdpackages/tempo/package.jsonpackages/tempo/src/engine/engine.normalizer.tspackages/tempo/src/support/support.init.tspackages/tempo/src/support/support.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tempo.version.tspackages/tempo/test/discrete/parse.locale.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/tempo/package.json
- packages/tempo/src/tempo.version.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/tempo/doc/tempo.cookbook.md (1)
282-334:⚠️ Potential issue | 🟡 MinorAdd test coverage for the exact outputs of Arabic and Japanese Intl formatting examples.
The
.format()method overload does correctly acceptIntl.DateTimeFormatOptions(confirmed inpackages/tempo/src/module/module.format.tsline 16) and delegates to the nativeIntl.DateTimeFormatAPI. The implementation properly handles timezone and calendar constraints via the try-catch fallback pattern (lines 104–113).However, the existing test for the Arabic example (instance.format.test.ts lines 48–68) only verifies that formatting doesn't crash and returns a non-empty string—it does not validate the exact output
"الأربعاء، ٢٥ ديسمبر ٢٠٢٤"as claimed in the documentation. The Japanese Reiwa era example (lines 318–334) has no test coverage at all.To match the documentation's claims, add assertions that verify the exact formatted outputs for both examples, ensuring that the
numberingSystem: 'arab'andera: 'long'options produce the specific strings shown in the cookbook.🤖 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 282 - 334, The documentation examples claim specific formatted outputs for Arabic numerals (with numberingSystem: 'arab') and Japanese Reiwa era formatting (with era: 'long'), but the test coverage does not verify these exact strings. Update the existing Arabic formatting test (which currently only checks for non-empty output) to assert the exact expected output "الأربعاء، ٢٥ ديسمبر ٢٠٢٤", and add a new test case for the Japanese Reiwa era example to verify it produces "令和6年12月25日". Both tests should use the exact Intl.DateTimeFormatOptions objects shown in the documentation (with locale, timeZone, calendar, era, and numberingSystem as specified) to ensure the .format() method correctly handles these complex internationalization scenarios.packages/tempo/src/module/module.format.ts (1)
247-247:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLocale-array configs break term
:localeresolution.At Line 247,
config?.locale?.split('-')[0]assumeslocaleis a string.packages/tempo/src/tempo.type.tsnow allowslocale: string | string[], so array inputs throw here and silently bypass term localization.💡 Proposed fix
- const lang = config?.locale?.split('-')[0] ?? 'en'; + const localeInput = Array.isArray(config?.locale) ? config.locale[0] : config?.locale; + const lang = localeInput ? String(localeInput).split('-')[0] : 'en';🤖 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/module/module.format.ts` at line 247, The `lang` assignment at line 247 assumes `config?.locale` is always a string, but the type in tempo.type.ts now allows `locale: string | string[]`. When an array is passed, the `.split('-')[0]` call fails because arrays do not have a split method. Update the assignment to handle both string and array cases: if locale is an array, extract the first element and then split it on the hyphen; if it is a string, split it directly on the hyphen. This ensures the language code is correctly extracted regardless of whether locale is a string or array input.packages/tempo/doc/tempo.config.md (1)
171-175:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftOptions table is outdated and contradicts code examples in the same file.
Lines 171–175 list top-level
event,period,snippet, andlayoutoptions, but the AI summary states this is "Tempo 4.0.0 with breaking registry consolidation," and the actual code examples in this document (lines 214–224, 258, 263) useregistry.events,registry.periods,registry.layouts,registry.ignores. Additionally, theregistryentry in the table describes only{ formats?, locales? }but omits the nested keys (events,periods,snippets,layouts,ignores) shown in the type contract.This table will mislead developers upgrading to v4.0.0 who expect top-level options to work.
🔧 Proposed fix for the options table
Update the table entries to reflect the current v4.0.0 registry structure and remove or deprecate the old top-level entries:
-| `event` | `Record<string, string \| Function>` | Built-in aliases | Custom date aliases merged into the event registry. | -| `period` | `Record<string, string \| Function>` | Built-in aliases | Custom time aliases merged into the period registry. | -| `snippet` | `Record<string, string \| RegExp>` | Built-in snippets | Custom snippet patterns used to compose parse layouts. | -| `layout` | `Record<string, string \| RegExp>` | Built-in layouts | Custom parse layouts for date/time pattern matching. | -| `registry` | `{ formats?, locales? }` | Built-in registries | Internal dictionary mappings (e.g., custom format tokens or localization dictionaries). | +| `registry` | `{ events?, periods?, snippets?, layouts?, formats?, locales?, ignores? }` | Built-in registries | Custom data augmentation registries. `events` and `periods` for aliases, `snippets` and `layouts` for parse patterns, `formats` and `locales` for formatting/i18n, `ignores` for noise-word filtering. |Alternatively, add a deprecation row before the table:
::: warning **Breaking Change (v4.0.0)**: In Tempo v3.x, custom aliases and patterns were configured via top-level `event`, `period`, `snippet`, and `layout` options. These are now consolidated under the `registry` object. See [Section 5.1](`#custom-events-and-periods`) and [Section 5.3](`#noise-word-filtering`) for updated examples. :::🤖 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.config.md` around lines 171 - 175, The options table at lines 171–175 is outdated and contradicts the v4.0.0 code examples shown later in the same document. Remove the separate `event`, `period`, `snippet`, and `layout` rows from the table since these are now consolidated under the `registry` object in v4.0.0. Update the `registry` row to document all nested keys (`events`, `periods`, `snippets`, `layouts`, `ignores`) instead of only listing `{ formats?, locales? }`. Additionally, add a deprecation warning section before the table to clarify that top-level options from v3.x have been consolidated under `registry` to help users upgrading to v4.0.0.
🧹 Nitpick comments (2)
packages/tempo/src/support/support.init.ts (1)
240-255: 💤 Low valueReDoS mitigations are present but residual risk remains.
The static analysis flags
new RegExp(pattern)at line 252 for potential ReDoS. The existing defenses (500-char length limit at line 244, backtrack pattern detection at line 248) provide reasonable protection. However, crafted patterns under 500 chars without detectable nested quantifiers could still cause slowdowns.Since this processes user-provided
registry.snippetsconfiguration, the current mitigations strike a reasonable balance between usability and safety. Consider documenting that snippet patterns should be kept simple, or adding a timeout wrapper if snippets are ever sourced from untrusted input.🤖 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 240 - 255, The snippet pattern validation in the collect callback has ReDoS mitigations (length limit and backtrack detection) but residual risk remains from crafted patterns that evade these checks. Add documentation noting that snippet patterns should be kept simple to mitigate ReDoS risk, and consider implementing a timeout wrapper around the RegExp constructor call if snippets are ever sourced from untrusted configuration inputs to provide defense-in-depth against potential ReDoS attacks.Source: Linters/SAST tools
packages/tempo/doc/tempo.cookbook.md (1)
230-244: ⚡ Quick winClarify when and where the FORMATS registry shortcut is resolved.
The "Tired of typing
:locale?" tip (lines 230–244) shows a format string key'ui-date'being passed directly to.format(). Confirm the documentation clearly explains:
- How Tempo resolves string keys to registered format templates (is it automatic, or does it require a specific initialization order?).
- Whether this lookup is case-sensitive and scoped globally or per instance.
- What error or behavior occurs if a key is not found in the registry.
The current text is clear but could benefit from a brief note on resolution scope/error handling to prevent user confusion.
Consider adding a single sentence such as: "Format keys are resolved case-sensitively from the global
registry.formats, and throw an error if not found." This would make the feature more discoverable and less error-prone.🤖 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 230 - 244, The "Tired of typing `:locale`?" tip section shows how to register and use format shortcuts but lacks clarity on how Tempo resolves these string keys. Add a brief explanatory sentence after the code example in the tip that clarifies: format keys are resolved case-sensitively from the global registry.formats object, and an error is thrown if the requested key is not found in the registry. This additional detail will help users understand the lookup behavior and what to expect if they encounter resolution failures.
🤖 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`:
- Around line 13-14: The CHANGELOG.md breaking change notes in lines 13-14 claim
that Tempo.formats and the top-level formats configuration key are completely
removed, but these members still exist as deprecated in the codebase
(Tempo.formats at packages/tempo/src/tempo.class.ts around line 965 and
formats/locales in the Discovery interface at packages/tempo/src/tempo.type.ts
around line 339). Update the CHANGELOG.md bullets to accurately reflect the
current state by changing "completely removed" and "entirely removed" language
to indicate these members are deprecated but still available, or alternatively
remove the deprecated members from the actual source code files if full removal
is the intended state. Ensure the breaking change documentation matches the
actual shipped API surface.
In `@packages/tempo/doc/installation.md`:
- Line 233: The JSDelivr CDN URL on line 233 points to `@4` but the accompanying
text incorrectly states "Locks to major version 3". Update the descriptive text
that follows the CDN URL from "Locks to major version 3" to "Locks to major
version 4" to correctly reflect the major version pinning in the URL reference.
In `@packages/tempo/test/discrete/parse.locale.test.ts`:
- Line 36: Remove the `localize: true` option from the Tempo constructor calls
at lines 15, 22, and 28 in the parse.locale.test.ts file. The `localize: true`
option is not defined in the BaseOptions interface and should not be used. Keep
the other options like `locale` and `anchor` that are properly supported. This
makes the test consistent with lines 36, 42, 46, and 51 which already follow the
correct pattern of using only the supported options.
---
Outside diff comments:
In `@packages/tempo/doc/tempo.config.md`:
- Around line 171-175: The options table at lines 171–175 is outdated and
contradicts the v4.0.0 code examples shown later in the same document. Remove
the separate `event`, `period`, `snippet`, and `layout` rows from the table
since these are now consolidated under the `registry` object in v4.0.0. Update
the `registry` row to document all nested keys (`events`, `periods`, `snippets`,
`layouts`, `ignores`) instead of only listing `{ formats?, locales? }`.
Additionally, add a deprecation warning section before the table to clarify that
top-level options from v3.x have been consolidated under `registry` to help
users upgrading to v4.0.0.
In `@packages/tempo/doc/tempo.cookbook.md`:
- Around line 282-334: The documentation examples claim specific formatted
outputs for Arabic numerals (with numberingSystem: 'arab') and Japanese Reiwa
era formatting (with era: 'long'), but the test coverage does not verify these
exact strings. Update the existing Arabic formatting test (which currently only
checks for non-empty output) to assert the exact expected output "الأربعاء، ٢٥
ديسمبر ٢٠٢٤", and add a new test case for the Japanese Reiwa era example to
verify it produces "令和6年12月25日". Both tests should use the exact
Intl.DateTimeFormatOptions objects shown in the documentation (with locale,
timeZone, calendar, era, and numberingSystem as specified) to ensure the
.format() method correctly handles these complex internationalization scenarios.
In `@packages/tempo/src/module/module.format.ts`:
- Line 247: The `lang` assignment at line 247 assumes `config?.locale` is always
a string, but the type in tempo.type.ts now allows `locale: string | string[]`.
When an array is passed, the `.split('-')[0]` call fails because arrays do not
have a split method. Update the assignment to handle both string and array
cases: if locale is an array, extract the first element and then split it on the
hyphen; if it is a string, split it directly on the hyphen. This ensures the
language code is correctly extracted regardless of whether locale is a string or
array input.
---
Nitpick comments:
In `@packages/tempo/doc/tempo.cookbook.md`:
- Around line 230-244: The "Tired of typing `:locale`?" tip section shows how to
register and use format shortcuts but lacks clarity on how Tempo resolves these
string keys. Add a brief explanatory sentence after the code example in the tip
that clarifies: format keys are resolved case-sensitively from the global
registry.formats object, and an error is thrown if the requested key is not
found in the registry. This additional detail will help users understand the
lookup behavior and what to expect if they encounter resolution failures.
In `@packages/tempo/src/support/support.init.ts`:
- Around line 240-255: The snippet pattern validation in the collect callback
has ReDoS mitigations (length limit and backtrack detection) but residual risk
remains from crafted patterns that evade these checks. Add documentation noting
that snippet patterns should be kept simple to mitigate ReDoS risk, and consider
implementing a timeout wrapper around the RegExp constructor call if snippets
are ever sourced from untrusted configuration inputs to provide defense-in-depth
against potential ReDoS attacks.
🪄 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: 19bbf679-340f-49f1-b2c2-81ae61d9865b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
packages/tempo/.vitepress/config.tspackages/tempo/CHANGELOG.mdpackages/tempo/README.mdpackages/tempo/doc/architecture.mdpackages/tempo/doc/installation.mdpackages/tempo/doc/lazy-evaluation-pattern.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/doc/releases/v4.x.mdpackages/tempo/doc/sandbox-factory.mdpackages/tempo/doc/tempo.benchmarks.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/doc/tempo.layout.mdpackages/tempo/doc/tempo.locale.mdpackages/tempo/doc/tempo.month-day.mdpackages/tempo/doc/tempo.parse.mdpackages/tempo/doc/tempo.planner.mdpackages/tempo/importmap.jsonpackages/tempo/package.jsonpackages/tempo/public/bundle.index.htmlpackages/tempo/src/module/module.format.tspackages/tempo/src/module/module.parse.tspackages/tempo/src/support/support.init.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tempo.version.tspackages/tempo/test/core/config.test.tspackages/tempo/test/core/global-options.test.tspackages/tempo/test/core/sandbox-factory.test.tspackages/tempo/test/core/static.getters.test.tspackages/tempo/test/discrete/format.test.tspackages/tempo/test/discrete/parse.locale.test.tspackages/tempo/test/discrete/standalone_parse.test.tspackages/tempo/test/engine/functional_alias_chaining.test.tspackages/tempo/test/instance/relative_date.test.tspackages/tempo/test/issues/infinite-loop.test.tspackages/tempo/test/issues/issue-fixes.test.ts
✅ Files skipped from review due to trivial changes (7)
- packages/tempo/doc/tempo.planner.md
- packages/tempo/doc/releases/v4.x.md
- packages/tempo/README.md
- packages/tempo/doc/releases/v2.x.md
- packages/tempo/doc/tempo.locale.md
- packages/tempo/doc/tempo.benchmarks.md
- packages/tempo/doc/lazy-evaluation-pattern.md
Summary by CodeRabbit
.format()to accept nativeIntl.DateTimeFormatOptions, includingtimeZone/calendar.{nano}support with BigInt-preserving epoch nanoseconds.locale: string | string[].