standalone format#19
Merged
magmacomputing merged 7 commits intomainfrom Apr 22, 2026
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 21 minutes and 0 seconds. ⌛ 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 (6)
📝 WalkthroughWalkthroughRefactors Tempo by extracting parse/format into standalone discrete modules, updates exports/imports/path aliases and package versions to 2.4.0, adds documentation and tests, and adjusts build/test config and VitePress settings to expose the new discrete entry points. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DiscreteModule as Discrete parse/format
participant Runtime as Runtime/State
participant Engine as ParseEngine/Formatter
participant Temporal as Temporal API
Client->>DiscreteModule: call parse(value, options) / format(value?, fmt?)
DiscreteModule->>Runtime: init()/markConfig(), extendState(), setPatterns()
DiscreteModule->>Engine: ParseEngine.parse(state, value) / render(template, zdt, state)
Engine->>Temporal: normalize Instant → ZonedDateTime, apply timezone/calendar
Engine-->>DiscreteModule: parsed ZonedDateTime / rendered string/number
DiscreteModule-->>Client: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tempo/package.json (1)
25-32:⚠️ Potential issue | 🟡 MinorRemove stale
sideEffectsentry fortempo.parse.jsand add discrete entry points.The
parsefunction has moved fromtempo.parse.jsto#tempo/parse(which maps to./dist/discrete/discrete.parse.js). Sincetempo.parse.jsis no longer emitted, this glob is dead. Additionally,discrete.parse.jsanddiscrete.format.jsinvokeinit()andgetRuntime(), making them side-effectful and requiring an entry insideEffects.Suggested update
"sideEffects": [ "**/temporal.polyfill.js", "**/*-polyfill.ts", "**/module.*.js", "**/module.*.ts", "**/tempo.index.js", - "**/tempo.parse.js" + "**/discrete.*.js" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/package.json` around lines 25 - 32, Update package.json's "sideEffects" array by removing the stale glob entry that matches "tempo.parse.js" (since the parse export moved to the "#tempo/parse" discrete entry) and instead add explicit entries for the discrete side-effectful bundles: include "./dist/discrete/discrete.parse.js" and "./dist/discrete/discrete.format.js" (or their package-relative equivalents) because these files call init() and getRuntime() and must be treated as having side effects; ensure the new entries replace the removed tempo.parse.js pattern so bundlers consider those modules side-effectful while the dead glob is removed.
🧹 Nitpick comments (3)
packages/tempo/src/discrete/discrete.parse.ts (1)
48-49: Timezone/calendar extraction logic is duplicated verbatim at Lines 136–138.The same ID-unwrapping snippet (
isString(x) ? x : (x as any).id ?? (x as any).timeZoneId) appears at least three times in this file (Lines 48–49, 137–138). Consider extracting a small helper to keep the normalization consistent (and to make future changes toTemporal.TimeZone/Temporal.Calendarobject shapes one-line edits):♻️ Suggested helper
const tzId = (v: any): string => isString(v) ? v : v?.id ?? v?.timeZoneId; const calId = (v: any): string => isString(v) ? v : v?.id ?? v?.calendarId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/discrete/discrete.parse.ts` around lines 48 - 49, Extract the repeated ID-unwrapping logic into small helper functions (e.g., tzId(v) and calId(v)) placed near the top of the module and replace the duplicated expressions that set tz and cal (the const tz = ... and const cal = ... lines) and the other duplicated spots in this file with calls to those helpers; ensure the helpers accept any value and return a string by doing the same normalization (isString check then .id ?? .timeZoneId/.calendarId) so all temporal TimeZone/Calendar normalization uses the single helper.packages/tempo/package.json (1)
127-134: Dual public paths forparse/format(./parseand./module/parse).Both
./parse(Line 175) and./module/parse(Line 127), and similarly./formatand./module/format, resolve to the same./dist/discrete/discrete.*.js. Having two canonical specifiers for the same module makes it harder for consumers (and bundlers) to dedupe, and risks divergent import conventions in downstream code. If./module/parse/./module/formatare only kept for migration compatibility, consider adding a deprecation note in the migration guide or removing them in favor of the shorter./parse/./format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/package.json` around lines 127 - 134, The package.json currently exposes duplicate export paths "./module/parse" and "./parse" (and "./module/format" and "./format") that both resolve to "./dist/discrete/discrete.*.js"; remove the redundant "./module/parse" and "./module/format" export entries from package.json so consumers have a single canonical specifier ("./parse" and "./format"), or alternatively keep them but document them as deprecated in the migration guide/README; update any mentions of "./module/parse" and "./module/format" (search for those exact strings) and ensure package.json only exports "./parse" and "./format" as the primary paths.packages/tempo/doc/tempo.format.md (1)
13-13:{day}token used in example but absent from the token table.Line 13 uses
{day}(unpadded day), which is supported indiscrete.format.ts(case 'day': return zdt.day.toString();), but the "Supported Tokens" table (Lines 80–99) only documents{dd}. Please add a row for{day}(and consider{dow},{yw},{yyww},{hhmiss},{nano}which are also implemented but undocumented) for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/doc/tempo.format.md` at line 13, The docs example uses the undocumented {day} token; update the "Supported Tokens" table in tempo.format.md to add a row for {day} (unpadded day) matching the implementation in discrete.format.ts (case 'day': return zdt.day.toString()); also audit discrete.format.ts for other implemented-but-undocumented tokens (e.g., case 'dow', 'yw', 'yyww', 'hhmiss', 'nano') and add corresponding entries to the table describing their output format so the docs match the formatter implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/tempo/doc/tempo.format.md`:
- Around line 90-105: Docs and implementation disagree: the `{HH}` token is
padded in discrete.format.ts using pad(zdt.hour > 12 ? zdt.hour % 12 : zdt.hour
|| 12), so examples showing unpadded values ("3" / "3:30pm") are incorrect;
either update the docs examples to "03" / "03:30pm" to match the current padded
behavior or change the implementation to output unpadded 12-hour values by
removing the pad call (or altering that expression) so `{HH}` yields "3" and
"3:30pm". Use the `{HH}` token, the pad(...) expression, and discrete.format.ts
to locate and apply the change.
In `@packages/tempo/README.md`:
- Around line 47-51: The examples showing diff.iso and event.format are
non-deterministic because event is created from a runtime-relative date; update
the README to use a fixed, reproducible event (or explicitly state the reference
date) so the outputs (e.g., the value returned by event.until('xmas') and the
string from event.format('{mon} {day}, {yyyy}')) match the examples; locate
usages of event, diff and the calls event.until and event.format in the Quick
Start snippet and replace them with a deterministic date or annotate the
examples with the reference date used to generate the shown outputs.
In `@packages/tempo/src/discrete/discrete.format.ts`:
- Around line 72-74: The lookup assumes formats.has exists and will throw for
plain objects; update the condition around template initialization to only call
formats.has when it's a function (e.g., Map) and fallback to an object key check
otherwise. Specifically, in the template assignment that references
isString(fmt), formats and formats.has, replace the plain formats.has check with
a guarded check (typeof (formats as any).has === 'function' ? (formats as
any).has(fmt as string) : Object.prototype.hasOwnProperty.call(formats, fmt as
string)) and then index into formats accordingly when selecting the template.
- Around line 32-44: The module is missing the Temporal polyfill import so
calling Temporal.Now.zonedDateTimeISO(...) in the formatter can throw when no
earlier entrypoint installed the polyfill; add the same polyfill import used
elsewhere (importing the Temporal polyfill at the top of
packages/tempo/src/discrete/discrete.format.ts) so Temporal is defined before
the code that computes zdt (the block using isTempo, isZonedDateTime, isInstant
and Temporal.Now.zonedDateTimeISO).
- Line 85: The code calls replaceAll(new RegExp(Match.braces)) which drops the
original RegExp flags and throws a TypeError; update both sites that call
replaceAll(new RegExp(Match.braces)) (the one assigning result in
discrete.format.ts and the similar call in tempo.util.ts) to either use
String.prototype.replace with an explicit global RegExp (e.g.
template.replace(new RegExp(Match.braces, 'g'), ...) ) or recreate the RegExp
preserving/adding the 'g' flag before calling replaceAll, ensuring Match.braces
is used with the 'g' flag so the replacement does not fail at runtime.
In `@packages/tempo/src/discrete/discrete.parse.ts`:
- Around line 538-560: The local shadow state created in parse() only sets
config and parse via Object.create, which leaves required fields like
state.OPTION and state.ZONED_DATE_TIME undefined (breaking
conform()/isZonedDateTimeLike()) and shares nested objects (e.g.,
state.parse.snippet) with globalState allowing mutations; fix by fully
populating the Internal.State shape when constructing state (copy/assign all
required properties from globalState or init() so state.OPTION and
state.ZONED_DATE_TIME exist) and stop using Object.create for shared
containers—use shallow clones instead (e.g., state.config = markConfig({
...globalState.config }), state.parse = markConfig({ ...globalState.parse }) )
and then shallow-clone any nested mutable maps/objects before mutations in
setPatterns (e.g., state.parse.snippet = { ...state.parse.snippet } and clone
any pattern maps) so parse(), extendState, setPatterns, conform(), and
isZonedDateTimeLike() operate on an isolated state instance.
In `@packages/tempo/src/library.index.ts`:
- Around line 2-4: The docblock header and description are inconsistent: the
title uses "Magma Library" but the sentence still references "Tempo"; update the
docblock so the terminology is unified by replacing the phrase "Tempo" with
"Magma" in the description (look for the header "Magma Library" and the line
containing "Tempo" in the top comment) so the entry-point docs consistently
reference "Magma".
In `@packages/tempo/test/standalone.test.ts`:
- Around line 8-12: The test calls init({ timeZone: 'UTC' }) in beforeAll which
mutates shared runtime.state and can leak into other tests; to fix, capture the
previous state (e.g., const prevState = runtime.state or at least prevTimeZone =
runtime.state.timeZone) before calling init in beforeAll, and restore it in
afterAll (call init(prevState) or set runtime.state.timeZone = prevTimeZone) so
the global runtime.state is returned to its original value; update the test to
use beforeAll/afterAll paired restoration around the init call referencing init,
beforeAll, afterAll, and runtime.state.
---
Outside diff comments:
In `@packages/tempo/package.json`:
- Around line 25-32: Update package.json's "sideEffects" array by removing the
stale glob entry that matches "tempo.parse.js" (since the parse export moved to
the "#tempo/parse" discrete entry) and instead add explicit entries for the
discrete side-effectful bundles: include "./dist/discrete/discrete.parse.js" and
"./dist/discrete/discrete.format.js" (or their package-relative equivalents)
because these files call init() and getRuntime() and must be treated as having
side effects; ensure the new entries replace the removed tempo.parse.js pattern
so bundlers consider those modules side-effectful while the dead glob is
removed.
---
Nitpick comments:
In `@packages/tempo/doc/tempo.format.md`:
- Line 13: The docs example uses the undocumented {day} token; update the
"Supported Tokens" table in tempo.format.md to add a row for {day} (unpadded
day) matching the implementation in discrete.format.ts (case 'day': return
zdt.day.toString()); also audit discrete.format.ts for other
implemented-but-undocumented tokens (e.g., case 'dow', 'yw', 'yyww', 'hhmiss',
'nano') and add corresponding entries to the table describing their output
format so the docs match the formatter implementation.
In `@packages/tempo/package.json`:
- Around line 127-134: The package.json currently exposes duplicate export paths
"./module/parse" and "./parse" (and "./module/format" and "./format") that both
resolve to "./dist/discrete/discrete.*.js"; remove the redundant
"./module/parse" and "./module/format" export entries from package.json so
consumers have a single canonical specifier ("./parse" and "./format"), or
alternatively keep them but document them as deprecated in the migration
guide/README; update any mentions of "./module/parse" and "./module/format"
(search for those exact strings) and ensure package.json only exports "./parse"
and "./format" as the primary paths.
In `@packages/tempo/src/discrete/discrete.parse.ts`:
- Around line 48-49: Extract the repeated ID-unwrapping logic into small helper
functions (e.g., tzId(v) and calId(v)) placed near the top of the module and
replace the duplicated expressions that set tz and cal (the const tz = ... and
const cal = ... lines) and the other duplicated spots in this file with calls to
those helpers; ensure the helpers accept any value and return a string by doing
the same normalization (isString check then .id ?? .timeZoneId/.calendarId) so
all temporal TimeZone/Calendar normalization uses the single helper.
🪄 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: c8f25d92-4350-4229-a39e-d4656a02ccf1
📒 Files selected for processing (24)
package.jsonpackages/library/package.jsonpackages/tempo/.vitepress/config.tspackages/tempo/README.mdpackages/tempo/bin/tsconfig.jsonpackages/tempo/doc/migration-guide.mdpackages/tempo/doc/tempo.format.mdpackages/tempo/doc/tempo.parse.mdpackages/tempo/importmap.jsonpackages/tempo/package.jsonpackages/tempo/src/core.index.tspackages/tempo/src/discrete/discrete.format.tspackages/tempo/src/discrete/discrete.index.tspackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/library.index.tspackages/tempo/src/plugin/module/module.format.tspackages/tempo/src/support/tempo.runtime.tspackages/tempo/src/tempo.index.tspackages/tempo/src/tempo.parse.tspackages/tempo/src/tsconfig.jsonpackages/tempo/test/standalone.test.tspackages/tempo/test/tsconfig.jsonpackages/tempo/vitest.config.tsvitest.config.ts
💤 Files with no reviewable changes (2)
- packages/tempo/src/tempo.parse.ts
- packages/tempo/src/plugin/module/module.format.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/tempo/src/discrete/discrete.format.ts`:
- Around line 37-45: The format() normalization currently only handles Tempo,
Temporal.ZonedDateTime, Temporal.Instant and undefined (see isTempo,
isZonedDateTime, isInstant branches) but the docs claim it also accepts ISO
strings, PlainDate and PlainDateTime; either implement normalization for those
types by extending the zdt resolution to detect string inputs (parse with
Temporal.ZonedDateTime.from or Temporal.PlainDate/PlainDateTime.from and then
convert to a ZonedDateTime with the provided tz), and add tests for
string/PlainDate/PlainDateTime paths, or update the documentation to remove ISO
string/PlainDate/PlainDateTime from supported inputs; ensure changes reference
the format() function and the zdt resolution logic so behavior matches docs and
tests.
🪄 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: f4247519-9ede-41b2-ab26-cb4fbe38d34a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
packages/tempo/.vitepress/config.tspackages/tempo/README.mdpackages/tempo/doc/lazy-evaluation-pattern.mdpackages/tempo/doc/migration-guide.mdpackages/tempo/doc/tempo.benchmarks.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.format.mdpackages/tempo/package.jsonpackages/tempo/src/discrete/discrete.format.tspackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/library.index.tspackages/tempo/src/support/tempo.util.tspackages/tempo/test/standalone.test.ts
💤 Files with no reviewable changes (1)
- packages/tempo/doc/lazy-evaluation-pattern.md
✅ Files skipped from review due to trivial changes (6)
- packages/tempo/doc/tempo.benchmarks.md
- packages/tempo/doc/tempo.config.md
- packages/tempo/doc/migration-guide.md
- packages/tempo/src/library.index.ts
- packages/tempo/README.md
- packages/tempo/test/standalone.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tempo/.vitepress/config.ts
Owner
Author
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
Owner
Author
|
standalone format |
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.
Standalone format() functionality
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests
Chores