Feature/tidy#60
Merged
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughTempo 3.9.0 adds historical-era parsing and formatting, new era getters, five community plugins, shared plugin build tooling, documentation, tests, release notes, and updated package metadata. ChangesTempo core and release metadata
Community plugins
Plugin tooling and documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Tempo
participant ParsePlanner
participant PluginAPI
participant PluginWorker
participant SharedClock
Tempo->>ParsePlanner: classify and prioritize era-bearing input
ParsePlanner-->>Tempo: return normalized date
Tempo->>PluginAPI: install community plugin
PluginAPI->>PluginWorker: dispatch batch work when applicable
PluginAPI->>SharedClock: start or read synchronized time when applicable
PluginWorker-->>Tempo: return transformed values
SharedClock-->>Tempo: return synchronized timestamp
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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: 5
🧹 Nitpick comments (1)
packages/tempo/test/engine/engine.era.test.ts (1)
3-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test cases for dotted era notations.
The
Token.eraregex insupport.default.tsis explicitly designed to handle dotted notation likeb.c.,b.c.e.,a.d., andc.e.. However, this test suite lacks coverage for these variations. Adding tests for them would ensure that dotted eras are correctly parsed and would help catch regressions (such as theERA_REplanner bug and the missing elements in theGuardarray).🧪 Proposed test additions
Consider adding tests similar to the following:
test('parses year and era with dots (trailing b.c.)', () => { const t = new Tempo('200 b.c.'); expect(t.yy).toBe(-199); expect(t.era).toMatch(/gregory-inverse/i); }); test('parses year and era with dots (trailing a.d.)', () => { const t = new Tempo('1 a.d.'); expect(t.yy).toBe(1); expect(t.era).toMatch(/gregory/i); });🤖 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.era.test.ts` around lines 3 - 62, Add dotted-era coverage to the “Era Parsing Engine” suite by testing trailing “b.c.” and “a.d.” inputs through Tempo, asserting the expected astronomical year and Gregorian or inverse-Gregorian era. Use the existing year/era test patterns and include the dotted variants explicitly.
🤖 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-fns/doc/functions/scheduling/index.md`:
- Around line 38-46: Update the scheduling example to use Temporal.Instant or
Temporal.ZonedDateTime for the start, end, meetingStart, and meetingEnd values
passed to Interval, matching the supported boundary types and the README.md
example; do not use Temporal.PlainDateTime.
In `@packages/tempo/src/engine/engine.lexer.ts`:
- Around line 179-183: Update the BCE detection regex in the era-handling block
to match dotted abbreviations such as “b.c.” and “b.c.e.” in addition to
existing BCE/BC forms, preserving case-insensitive matching and the current
negative-year adjustment through yy.
In `@packages/tempo/src/engine/engine.planner.ts`:
- Line 6: Update ERA_RE in packages/tempo/src/engine/engine.planner.ts:6 to
match dotted and undotted era forms without accepting substrings; extend the
Guard array in packages/tempo/src/support/support.default.ts:197-198 with dotted
variants; and add regression cases for values such as “200 b.c.” and “1 a.d.” in
packages/tempo/test/engine/engine.era.test.ts:3-62.
In `@packages/tempo/src/module/module.format.ts`:
- Around line 292-296: Update the `dots` case in the token formatting switch for
`mer` and `era` so existing periods are removed from the evaluated `res` string
before inserting separators and the trailing period. Preserve the current dotted
output for values without periods while preventing consecutive dots for
locale-provided values such as `A.D.`.
In `@packages/tempo/src/support/support.default.ts`:
- Around line 197-198: Add the dotted era aliases b.c., b.c.e., a.d., and c.e.
to the guard list alongside the existing dotless era entries, so Guard permits
all forms already accepted by Token.era.
---
Nitpick comments:
In `@packages/tempo/test/engine/engine.era.test.ts`:
- Around line 3-62: Add dotted-era coverage to the “Era Parsing Engine” suite by
testing trailing “b.c.” and “a.d.” inputs through Tempo, asserting the expected
astronomical year and Gregorian or inverse-Gregorian era. Use the existing
year/era test patterns and include the dotted variants explicitly.
🪄 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: 79d6f07f-de0b-44a5-b29b-2684c4364e6a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
CHANGELOG.mdpackage.jsonpackages/library/package.jsonpackages/tempo-fns/doc/functions/scheduling/index.mdpackages/tempo-fns/src/scheduling/README.mdpackages/tempo/.vitepress/config.tspackages/tempo/CHANGELOG.mdpackages/tempo/doc/2-core-concepts/tempo.config.mdpackages/tempo/doc/2-core-concepts/tempo.format.mdpackages/tempo/doc/2-core-concepts/tempo.getters.mdpackages/tempo/doc/2-core-concepts/tempo.parse.mdpackages/tempo/doc/8-project-and-support/releases/v3.x.mdpackages/tempo/package.jsonpackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/engine/engine.planner.tspackages/tempo/src/module/module.format.tspackages/tempo/src/support/support.default.tspackages/tempo/src/support/support.symbol.tspackages/tempo/src/tempo.class.tspackages/tempo/test/core/static.test.tspackages/tempo/test/discrete/format.test.tspackages/tempo/test/engine/engine.era.test.ts
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/src/engine/engine.lexer.ts (1)
179-188: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not discard eras without an explicit year.
Inputs like1 Jan BCstill reach this path withyyunset; deletinggroups["era"]letsfallbackYearturn them into the current/anchor year. Require a year for era-bearing dates, or reject them.🤖 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.lexer.ts` around lines 179 - 188, Update the era handling in the lexer path around the isBCE check so an era-bearing date without an explicit yy is not normalized using fallbackYear. Require yy before applying BCE conversion and deleting groups["era"], or reject the input when yy is unset; preserve existing behavior for dates that include a year.
🤖 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/src/engine/engine.lexer.ts`:
- Around line 179-188: Update the era handling in the lexer path around the
isBCE check so an era-bearing date without an explicit yy is not normalized
using fallbackYear. Require yy before applying BCE conversion and deleting
groups["era"], or reject the input when yy is unset; preserve existing behavior
for dates that include a year.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e69836b-9ddf-4721-98cd-7656f39e011a
📒 Files selected for processing (7)
packages/tempo-fns/doc/functions/scheduling/index.mdpackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/engine/engine.planner.tspackages/tempo/src/module/module.format.tspackages/tempo/src/support/support.default.tspackages/tempo/src/tempo.version.tspackages/tempo/test/engine/engine.era.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/tempo-fns/doc/functions/scheduling/index.md
- packages/tempo/test/engine/engine.era.test.ts
- packages/tempo/src/module/module.format.ts
- packages/tempo/src/support/support.default.ts
- packages/tempo/src/engine/engine.planner.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
packages/plugins/snap/src/index.ts-64-64 (1)
64-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude all lower-order fields in fractional values.
For example, snapping
14:00:00.500upward by one hour currently remains at14:00, because milliseconds are omitted. Hours must include milliseconds/microseconds/nanoseconds; minutes and seconds have equivalent omissions.Also applies to: 77-77, 90-90
🤖 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/plugins/snap/src/index.ts` at line 64, Update the fractional time calculations in the snapping logic, including the expressions at the referenced hour, minute, and second calculations, to include every available lower-order field. Ensure hours incorporate minutes, seconds, milliseconds, microseconds, and nanoseconds; minutes incorporate seconds and finer units; and seconds incorporate milliseconds, microseconds, and nanoseconds, preserving the existing snapping behavior otherwise.packages/plugins/snap/src/index.ts-40-52 (1)
40-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-finite snap steps.
NaNandInfinitypass the zero check and propagate invalid values intoadd. Validate the raw step before taking its absolute value.Proposed fix
const key = providedKeys[0]; let step = opts[key as keyof SnapOptions] as number; - if (step === 0) { - const err = new Error(`Snap step cannot be zero.`); + if (!Number.isFinite(step) || step === 0) { + const err = new Error(`Snap step must be a finite, non-zero number.`);🤖 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/plugins/snap/src/index.ts` around lines 40 - 52, Update the snap-step validation in the shown method to reject any non-finite raw step, including NaN and Infinity, before calling Math.abs. Reuse the existing catch-and-return or throw error flow for invalid values, while preserving valid nonzero step handling.packages/plugins/snap/test/snap.test.ts-6-8 (1)
6-8: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRegister the plugin outside an individual test.
Later tests depend on this side effect and fail when run alone with a test-name filter. Use
beforeAllor suite-level initialization.Proposed fix
+beforeAll(() => { + Tempo.extend(MutateModule, FormatModule, SnapPlugin); +}); + describe('Snap Plugin', () => { it('should snap to the nearest 15 minutes', () => { - Tempo.extend(MutateModule, FormatModule, SnapPlugin); -🤖 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/plugins/snap/test/snap.test.ts` around lines 6 - 8, Move the Tempo.extend(MutateModule, FormatModule, SnapPlugin) registration out of the individual test and into suite-level initialization such as beforeAll within the “Snap Plugin” describe block, so every test receives the plugin setup even when run alone.packages/plugins/bin/repl.mts-44-45 (1)
44-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
pathToFileURLfor the dynamic import.file://${indexPath}breaks on Windows drive-letter paths;pathToFileURL(indexPath).hrefis the cross-platform form.🤖 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/plugins/bin/repl.mts` around lines 44 - 45, Update the dynamic import in the module-loading flow to use pathToFileURL(indexPath).href instead of manually constructing a file:// URL, ensuring Windows drive-letter and Linux paths are handled correctly.packages/plugins/tsup.shared.ts-58-64 (1)
58-64: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake the injected package version authoritative.
...configcurrently overwrites the generatedversion, allowing stale source metadata to defeat auto-injection.Proposed fix
-contents += `export const definePlugin = (config) => originalPlugin({ version: "${version}", ...config });\n`; -contents += `export const defineTerm = (config) => originalTerm({ version: "${version}", ...config });\n`; +contents += `export const definePlugin = (config) => originalPlugin({ ...config, version: "${version}" });\n`; +contents += `export const defineTerm = (config) => originalTerm({ ...config, version: "${version}" });\n`; ... -contents += `export const definePlugin = (config) => originalPlugin({ version: "${version}", ...config });\n`; +contents += `export const definePlugin = (config) => originalPlugin({ ...config, version: "${version}" });\n`;🤖 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/plugins/tsup.shared.ts` around lines 58 - 64, Update the generated definePlugin and defineTerm wrappers in the isApi branch so the injected version remains authoritative: spread config before assigning version. Apply the same ordering to the non-API definePlugin wrapper, preserving the existing imports and wrapper behavior.packages/plugins/astro/src/index.ts-7-24 (1)
7-24: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDeclare the returned
strictfield inTempoTermRegistry.The scoped result exposes
strict, but the publicastronomytype omits it, causing valid consumer access to fail type checking.Proposed fix
end: Tempo; + strict?: 'Vernal' | 'Summer' | 'Autumnal' | 'Winter'; }Also applies to: 161-164
🤖 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/plugins/astro/src/index.ts` around lines 7 - 24, Update the public astronomy result type in TempoTermRegistry to declare the returned strict field, matching the scoped result shape and allowing consumers to access it without type errors. Apply the same addition to the corresponding astronomy type declaration referenced by the comment.packages/plugins/batch/test/batch.test.ts-15-16 (1)
15-16: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIncorrect arguments passed to
Tempo.batch.The
batchfunction is defined inpackages/plugins/batch/src/index.tswith the signaturebatch(epochs: number[], operation: string, options?: BatchOptions). Calling it with two arguments where the second is an object (e.g.,{ weeks: 1 }) omits the requiredoperationstring and will result in runtime errors once this test is enabled.🤖 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/plugins/batch/test/batch.test.ts` around lines 15 - 16, Update the Tempo.batch invocation in the batch test to pass the required operation string as the second argument and move the `{ weeks: 1 }` value into the optional options argument. Preserve the existing epochs input and ensure the call matches the signature defined by batch.packages/plugins/batch/test/benchmark.ts-32-33 (1)
32-33: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIncorrect arguments passed to
Tempo.batch.The TS ignore hides the fact that
Tempo.batchis being called incorrectly. Thebatchmethod requires anoperationstring as its second argument (e.g.,'add'), but here an object is passed instead.🤖 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/plugins/batch/test/benchmark.ts` around lines 32 - 33, Update the Tempo.batch invocation in the benchmark to pass the required operation string as its second argument, followed by the existing options object in the correct parameter position. Remove the `@ts-ignore` directive so TypeScript validates the corrected call.packages/plugins/sync/package.json-4-4 (1)
4-4: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove
private: trueif this package should be published.This plugin is configured with
publishConfig,prepublishOnlyscripts, andplan: "community", which strongly implies it is intended to be published to npm. However, theprivate: trueflag will completely preventnpm publishfrom executing.If publishing is intended, remove the
privatefield.🛠️ Proposed fix
"version": "1.0.0", - "private": true, "description": "Tempo community plugin providing lock-free, highly precise cross-thread synchronization via SharedArrayBuffer and Atomics.",🤖 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/plugins/sync/package.json` at line 4, Remove the private field from the sync package manifest so its existing publishConfig, prepublishOnly scripts, and community plan can support npm publication.packages/plugins/sync/src/AtomicClock.ts-27-28 (1)
27-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix unit discrepancy in the comment.
The comment indicates the buffer stores the epoch in milliseconds, but the
#tick()implementation below clearly stores epoch nanoseconds. Since this buffer is designed to be shared with worker threads, the unit must be accurately documented to prevent downstream consumers from misinterpreting the time.📝 Proposed fix
- // Allocate 8 bytes for a 64-bit integer (epoch in milliseconds) + // Allocate 8 bytes for a 64-bit integer (epoch in nanoseconds) this.#buffer = new SharedArrayBuffer(8);🤖 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/plugins/sync/src/AtomicClock.ts` around lines 27 - 28, Update the allocation comment in the AtomicClock constructor to state that the 64-bit integer stores epoch nanoseconds, matching the unit written by `#tick`().
🧹 Nitpick comments (1)
packages/plugins/batch/package.json (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a cross-platform command for file removal.
The
rm -rfcommand is Unix-specific and will fail on Windows environments (e.g., standard Command Prompt or PowerShell). Consider using a cross-platform utility likerimraf,shx rm -rf, or a short Node.js script usingfs.rmSyncto ensure the build script is platform-agnostic.♻️ Proposed fix using `fs`
- "postbuild": "rm -rf dist/src", + "postbuild": "node -e \"fs.rmSync('dist/src', { recursive: true, force: true })\"",🤖 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/plugins/batch/package.json` at line 21, Update the package.json postbuild script to remove dist/src using a cross-platform deletion utility or Node.js fs.rmSync implementation instead of the Unix-specific rm -rf command, while preserving the existing cleanup behavior.
🤖 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/plugins/batch/CHANGELOG.md`:
- Line 8: Align the batch plugin’s licensing classification across both affected
sites: update the release description in packages/plugins/batch/CHANGELOG.md
lines 8-8 from “premium” to the confirmed classification, and adjust the usage
and token requirements in packages/plugins/batch/doc/index.md lines 54-58 to
match it. Use one authoritative classification consistently in both documents.
In `@packages/plugins/batch/src/BatchOrchestrator.ts`:
- Around line 18-21: Update BatchOrchestrator’s public options and execution
paths so formatting and rehydrate are only exposed when fully supported: route
nonnumeric formatting operations through postMessage instead of Float64Array,
and apply rehydrate consistently in both SharedArrayBuffer and fallback paths;
otherwise remove these options and their documentation until implemented.
- Around line 35-40: Validate options.threads in BatchOrchestrator.transform
before calculating chunkSize, rejecting values that are non-positive or
non-integer while preserving the default CPU-count behavior when omitted. Ensure
invalid thread counts fail before worker creation or either execution path.
In `@packages/plugins/batch/src/worker.ts`:
- Around line 20-22: In both the sab and postMessage loops in worker.ts, update
the Tempo handling so each `(t as any).add(operation)` result is assigned to a
variable, then read that result’s epoch.ms for outputView instead of reading
from the original immutable Tempo instance.
In `@packages/plugins/bin/repl.mts`:
- Around line 22-25: Update the mock revocation setup around mockToken so it
runs only when the explicit test-mode variable is enabled, not merely when
TEMPO_LICENSE_KEY is supplied. Preserve the existing default assignments for
TEMPO_REVOCATION_URL and TEMPO_REVOCATION_JWS within that test-mode branch.
- Around line 31-33: Update the packagesDir path used by the plugin discovery
flow in the REPL script so it resolves to the sibling packages directory from
packages/plugins/bin, not packages/plugins/packages. Keep the existing
fs.readdirSync discovery behavior unchanged after correcting the path.
In `@packages/plugins/bin/tsconfig.json`:
- Line 2: Update the extends path in the plugins bin tsconfig to reference
../../tsconfig.base.json so it resolves to the repository base configuration
instead of the missing packages/plugins path.
In `@packages/plugins/finance/package.json`:
- Around line 28-34: Align the `@magmacomputing/tempo` entry in devDependencies
with its peerDependencies version by updating it from ^3.6.1 to at least ^3.8.0,
ensuring local development and tests use a compatible release.
In `@packages/plugins/finance/src/index.ts`:
- Around line 27-34: Update the Tempo module augmentation for the finance
property so it is not required on every Tempo instance. Prefer marking finance
optional, or define and export a plugin-specific TempoWithFinance type, while
preserving the existing finance members for instances extended with
FinancePlugin.
In `@packages/plugins/sync/test/sync.test.ts`:
- Around line 7-29: Ensure the clock created in the test is always stopped by
wrapping the test body after AtomicClock initialization in a try/finally block
and moving clock.stop() into the finally clause. Preserve the existing
assertions and cleanup behavior while guaranteeing cleanup when any assertion or
awaited operation fails.
In `@packages/plugins/tsup.shared.ts`:
- Around line 81-94: Update the IIFE globals mapping and resolver around the
globals object and build.onResolve so `@magmacomputing/tempo/plugin` resolves to
its intended window global instead of window.undefined. Add an explicit mapping
for this specifier, or narrow the resolver filter to only mapped specifiers
while preserving the existing mappings.
- Around line 39-42: Update the auto-injected-version branch in the
build.onResolve handler for `@magmacomputing/tempo/plugin` imports to continue
through the remaining resolvers instead of returning external: true. Preserve
the resolver ordering so license-alias runs before iife-globals, while
preventing the resolver cycle without leaving a runtime import or bypassing the
license wrapper.
---
Minor comments:
In `@packages/plugins/astro/src/index.ts`:
- Around line 7-24: Update the public astronomy result type in TempoTermRegistry
to declare the returned strict field, matching the scoped result shape and
allowing consumers to access it without type errors. Apply the same addition to
the corresponding astronomy type declaration referenced by the comment.
In `@packages/plugins/batch/test/batch.test.ts`:
- Around line 15-16: Update the Tempo.batch invocation in the batch test to pass
the required operation string as the second argument and move the `{ weeks: 1 }`
value into the optional options argument. Preserve the existing epochs input and
ensure the call matches the signature defined by batch.
In `@packages/plugins/batch/test/benchmark.ts`:
- Around line 32-33: Update the Tempo.batch invocation in the benchmark to pass
the required operation string as its second argument, followed by the existing
options object in the correct parameter position. Remove the `@ts-ignore`
directive so TypeScript validates the corrected call.
In `@packages/plugins/bin/repl.mts`:
- Around line 44-45: Update the dynamic import in the module-loading flow to use
pathToFileURL(indexPath).href instead of manually constructing a file:// URL,
ensuring Windows drive-letter and Linux paths are handled correctly.
In `@packages/plugins/snap/src/index.ts`:
- Line 64: Update the fractional time calculations in the snapping logic,
including the expressions at the referenced hour, minute, and second
calculations, to include every available lower-order field. Ensure hours
incorporate minutes, seconds, milliseconds, microseconds, and nanoseconds;
minutes incorporate seconds and finer units; and seconds incorporate
milliseconds, microseconds, and nanoseconds, preserving the existing snapping
behavior otherwise.
- Around line 40-52: Update the snap-step validation in the shown method to
reject any non-finite raw step, including NaN and Infinity, before calling
Math.abs. Reuse the existing catch-and-return or throw error flow for invalid
values, while preserving valid nonzero step handling.
In `@packages/plugins/snap/test/snap.test.ts`:
- Around line 6-8: Move the Tempo.extend(MutateModule, FormatModule, SnapPlugin)
registration out of the individual test and into suite-level initialization such
as beforeAll within the “Snap Plugin” describe block, so every test receives the
plugin setup even when run alone.
In `@packages/plugins/sync/package.json`:
- Line 4: Remove the private field from the sync package manifest so its
existing publishConfig, prepublishOnly scripts, and community plan can support
npm publication.
In `@packages/plugins/sync/src/AtomicClock.ts`:
- Around line 27-28: Update the allocation comment in the AtomicClock
constructor to state that the 64-bit integer stores epoch nanoseconds, matching
the unit written by `#tick`().
In `@packages/plugins/tsup.shared.ts`:
- Around line 58-64: Update the generated definePlugin and defineTerm wrappers
in the isApi branch so the injected version remains authoritative: spread config
before assigning version. Apply the same ordering to the non-API definePlugin
wrapper, preserving the existing imports and wrapper behavior.
---
Nitpick comments:
In `@packages/plugins/batch/package.json`:
- Line 21: Update the package.json postbuild script to remove dist/src using a
cross-platform deletion utility or Node.js fs.rmSync implementation instead of
the Unix-specific rm -rf command, while preserving the existing cleanup
behavior.
🪄 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: 84918005-7745-4b0f-9cae-819c9e80ffcc
⛔ Files ignored due to path filters (11)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/plugins/astro/.turbo/turbo-build.logis excluded by!**/*.logpackages/plugins/astro/.turbo/turbo-test.logis excluded by!**/*.logpackages/plugins/batch/.turbo/turbo-build.logis excluded by!**/*.logpackages/plugins/batch/.turbo/turbo-test.logis excluded by!**/*.logpackages/plugins/finance/.turbo/turbo-build.logis excluded by!**/*.logpackages/plugins/finance/.turbo/turbo-test.logis excluded by!**/*.logpackages/plugins/snap/.turbo/turbo-build.logis excluded by!**/*.logpackages/plugins/snap/.turbo/turbo-test.logis excluded by!**/*.logpackages/plugins/sync/.turbo/turbo-build.logis excluded by!**/*.logpackages/plugins/sync/.turbo/turbo-test.logis excluded by!**/*.log
📒 Files selected for processing (78)
package.jsonpackages/plugins/astro/CHANGELOG.mdpackages/plugins/astro/LICENSEpackages/plugins/astro/README.mdpackages/plugins/astro/doc/index.mdpackages/plugins/astro/package.jsonpackages/plugins/astro/src/index.tspackages/plugins/astro/test/astro.test.tspackages/plugins/astro/test/tsconfig.jsonpackages/plugins/astro/tsconfig.jsonpackages/plugins/astro/tsup.config.tspackages/plugins/batch/CHANGELOG.mdpackages/plugins/batch/LICENSEpackages/plugins/batch/README.mdpackages/plugins/batch/doc/index.mdpackages/plugins/batch/package.jsonpackages/plugins/batch/src/BatchOrchestrator.tspackages/plugins/batch/src/index.tspackages/plugins/batch/src/worker.tspackages/plugins/batch/test/batch.test.tspackages/plugins/batch/test/benchmark.tspackages/plugins/batch/test/polyfill.tspackages/plugins/batch/test/tsconfig.jsonpackages/plugins/batch/tsconfig.jsonpackages/plugins/batch/tsup.config.tspackages/plugins/bin/repl.mtspackages/plugins/bin/temporal-polyfill.mtspackages/plugins/bin/tsconfig.jsonpackages/plugins/finance/README.mdpackages/plugins/finance/doc/index.mdpackages/plugins/finance/package.jsonpackages/plugins/finance/src/index.tspackages/plugins/finance/test/finance.test.tspackages/plugins/finance/test/tsconfig.jsonpackages/plugins/finance/tsconfig.jsonpackages/plugins/finance/tsup.config.tspackages/plugins/snap/CHANGELOG.mdpackages/plugins/snap/LICENSEpackages/plugins/snap/README.mdpackages/plugins/snap/doc/index.mdpackages/plugins/snap/package.jsonpackages/plugins/snap/src/index.tspackages/plugins/snap/test/snap.test.tspackages/plugins/snap/test/tsconfig.jsonpackages/plugins/snap/tsconfig.jsonpackages/plugins/snap/tsup.config.tspackages/plugins/sync/CHANGELOG.mdpackages/plugins/sync/LICENSEpackages/plugins/sync/README.mdpackages/plugins/sync/doc/index.mdpackages/plugins/sync/package.jsonpackages/plugins/sync/src/AtomicClock.tspackages/plugins/sync/src/AtomicReader.tspackages/plugins/sync/src/index.tspackages/plugins/sync/test/sync.test.tspackages/plugins/sync/test/tsconfig.jsonpackages/plugins/sync/tsconfig.jsonpackages/plugins/sync/tsup.config.tspackages/plugins/ticker/doc/index.mdpackages/plugins/tsconfig.shared.jsonpackages/plugins/tsup.shared.tspackages/tempo/bin/harvest-plugins.mjspackages/tempo/doc/2-core-concepts/tempo.format.mdpackages/tempo/doc/2-core-concepts/tempo.getters.mdpackages/tempo/doc/2-core-concepts/tempo.layout.mdpackages/tempo/doc/8-project-and-support/releases/v3.x.mdpackages/tempo/doc/9-plugins/astro.mdpackages/tempo/doc/9-plugins/batch.mdpackages/tempo/doc/9-plugins/finance.mdpackages/tempo/doc/9-plugins/snap.mdpackages/tempo/doc/9-plugins/sync.mdpackages/tempo/doc/9-plugins/ticker.mdpackages/tempo/package.jsonpackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/module/module.format.tspackages/tempo/src/tempo.class.tspackages/tempo/test/core/static.test.tsvitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/tempo/doc/2-core-concepts/tempo.getters.md
- packages/tempo/test/core/static.test.ts
- packages/tempo/src/engine/engine.lexer.ts
- packages/tempo/package.json
- packages/tempo/doc/2-core-concepts/tempo.format.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/plugins/astro/src/index.ts (1)
40-53: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftComplete the season calculation. This only uses the mean JDE polynomial; the Meeus approximation also adds higher-order terms and periodic corrections. If you intend to keep this approximation, document/enforce the supported year range and add regression fixtures for boundary years.
🤖 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/plugins/astro/src/index.ts` around lines 40 - 53, Update calculateAstroMoment to complete the Meeus seasonal calculation by including the required higher-order terms and periodic corrections for each ASTRO quarter before converting JDE to epoch milliseconds. If retaining the current mean-polynomial approximation instead, document and enforce its supported year range and add regression fixtures covering the range boundaries.
🧹 Nitpick comments (3)
packages/tempo/rollup.config.js (2)
104-119: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse async
transformfor better build performance.Make the hook
asyncand await thetransformpromise to avoid blocking the event loop.⚡ Proposed refactor for the transform hook
- transform(code, id) { + async transform(code, id) { if (!id.endsWith('.ts')) return null; try { - const result = transformSync(code, { + const result = await transform(code, { loader: 'ts', target: 'esnext', format: 'esm', sourcemap: false }); return { code: result.code, map: null }; } catch (err) { this.error(`esbuild compilation failed in ${id}:\n${err.message}`); }🤖 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/rollup.config.js` around lines 104 - 119, Update the transform hook containing transformSync to be async, replace the synchronous transform call with the asynchronous transform API, and await its promise before returning the generated code and map. Preserve the existing options and error handling in the hook.
7-7: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse async
transformfor better build performance.Rollup calls the
transformhook concurrently for multiple modules. UsingtransformSyncblocks the Node.js event loop, which prevents Rollup from processing files in parallel. Switching to esbuild's asynchronoustransformAPI allows for concurrent transpilation and can significantly speed up the build.⚡ Proposed refactor to use the async API
-import { transformSync } from 'esbuild'; +import { transform } from 'esbuild';🤖 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/rollup.config.js` at line 7, Replace the synchronous esbuild transform import and usage with the asynchronous transform API in the Rollup transform hook, updating the hook to await the returned result while preserving the existing transpilation options and output handling.packages/plugins/batch/src/BatchOrchestrator.ts (1)
103-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove stale
rehydratecomment.Since
rehydratelogic has been successfully extracted to the plugin layer (index.ts) and is no longer part of this method's responsibility, this comment is obsolete and can be removed.♻️ Proposed refactor
- // (Optional) Rehydrate logic would go here if options.rehydrate === true return result;🤖 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/plugins/batch/src/BatchOrchestrator.ts` around lines 103 - 104, Remove the obsolete rehydrate comment immediately before the return statement in BatchOrchestrator’s method, leaving the existing result return and plugin-layer rehydrate behavior unchanged.
🤖 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/magma-cli/index.js`:
- Around line 7-16: Update the `command === 'rm'` deletion loop so failures
caught around `fs.rmSync` are rethrown or otherwise propagated after logging,
causing `magma-cli rm` to exit nonzero and preventing subsequent `&&` build
steps from running; preserve the existing per-path removal behavior and error
context.
In `@packages/plugins/batch/src/BatchOrchestrator.ts`:
- Around line 30-31: Add rehydrate?: boolean to the BatchOptions interface used
by BatchOrchestrator.transform, while retaining the existing threads?: number
property, so index.ts can safely access options?.rehydrate without TypeScript
errors.
In `@packages/plugins/batch/test/batch.test.ts`:
- Around line 15-18: Update the batch invocation in the test scaffold to pass a
valid mutation string such as “+1w” as the operation argument, while preserving
the existing epochs input and options object. Use the batch call around
Tempo.batch; do not pass “add” because the worker forwards the operation
directly to t.add(operation).
In `@packages/tempo/README.md`:
- Around line 20-29: Replace the inline-styled flex container around the badges
with a <p align="center"> wrapper, preserving all existing badge links and
images while removing unsupported flexbox and gap styling.
---
Outside diff comments:
In `@packages/plugins/astro/src/index.ts`:
- Around line 40-53: Update calculateAstroMoment to complete the Meeus seasonal
calculation by including the required higher-order terms and periodic
corrections for each ASTRO quarter before converting JDE to epoch milliseconds.
If retaining the current mean-polynomial approximation instead, document and
enforce its supported year range and add regression fixtures covering the range
boundaries.
---
Nitpick comments:
In `@packages/plugins/batch/src/BatchOrchestrator.ts`:
- Around line 103-104: Remove the obsolete rehydrate comment immediately before
the return statement in BatchOrchestrator’s method, leaving the existing result
return and plugin-layer rehydrate behavior unchanged.
In `@packages/tempo/rollup.config.js`:
- Around line 104-119: Update the transform hook containing transformSync to be
async, replace the synchronous transform call with the asynchronous transform
API, and await its promise before returning the generated code and map. Preserve
the existing options and error handling in the hook.
- Line 7: Replace the synchronous esbuild transform import and usage with the
asynchronous transform API in the Rollup transform hook, updating the hook to
await the returned result while preserving the existing transpilation options
and output handling.
🪄 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: 3d34490f-1a4e-488d-a3b1-616d6263d976
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (64)
packages/magma-cli/README.mdpackages/magma-cli/index.jspackages/magma-cli/package.jsonpackages/plugins/astro/README.mdpackages/plugins/astro/doc/index.mdpackages/plugins/astro/package.jsonpackages/plugins/astro/src/index.tspackages/plugins/astro/test/tsconfig.jsonpackages/plugins/astro/tsconfig.jsonpackages/plugins/batch/CHANGELOG.mdpackages/plugins/batch/README.mdpackages/plugins/batch/doc/index.mdpackages/plugins/batch/package.jsonpackages/plugins/batch/src/BatchOrchestrator.tspackages/plugins/batch/src/worker.tspackages/plugins/batch/test/batch.test.tspackages/plugins/batch/test/benchmark.tspackages/plugins/batch/test/tsconfig.jsonpackages/plugins/batch/tsconfig.jsonpackages/plugins/bin/repl.mtspackages/plugins/bin/tsconfig.jsonpackages/plugins/finance/CHANGELOG.mdpackages/plugins/finance/LICENSEpackages/plugins/finance/README.mdpackages/plugins/finance/doc/index.mdpackages/plugins/finance/package.jsonpackages/plugins/finance/test/tsconfig.jsonpackages/plugins/finance/tsconfig.jsonpackages/plugins/snap/README.mdpackages/plugins/snap/doc/index.mdpackages/plugins/snap/package.jsonpackages/plugins/snap/src/index.tspackages/plugins/snap/test/snap.test.tspackages/plugins/snap/test/tsconfig.jsonpackages/plugins/snap/tsconfig.jsonpackages/plugins/snap/tsup.config.tspackages/plugins/sync/README.mdpackages/plugins/sync/doc/index.mdpackages/plugins/sync/package.jsonpackages/plugins/sync/src/AtomicClock.tspackages/plugins/sync/test/sync.test.tspackages/plugins/sync/test/tsconfig.jsonpackages/plugins/sync/tsconfig.jsonpackages/plugins/sync/tsup.config.tspackages/plugins/ticker/README.mdpackages/plugins/ticker/doc/index.mdpackages/plugins/tsconfig.test.jsonpackages/plugins/tsup.shared.tspackages/tempo-fns/package.jsonpackages/tempo/.vitepress/config.tspackages/tempo/.vitepress/theme/components/CatalogList.vuepackages/tempo/README.mdpackages/tempo/archive/tempo.api.mdpackages/tempo/archive/tempo.types.mdpackages/tempo/doc/1-getting-started/tempo.cookbook.mdpackages/tempo/doc/4-advanced-reference/tempo.ticker.mdpackages/tempo/doc/9-plugins/astro.mdpackages/tempo/doc/9-plugins/batch.mdpackages/tempo/doc/9-plugins/finance.mdpackages/tempo/doc/9-plugins/snap.mdpackages/tempo/doc/9-plugins/sync.mdpackages/tempo/doc/9-plugins/ticker.mdpackages/tempo/package.jsonpackages/tempo/rollup.config.js
💤 Files with no reviewable changes (8)
- packages/plugins/batch/tsconfig.json
- packages/plugins/batch/package.json
- packages/tempo/archive/tempo.types.md
- packages/plugins/astro/package.json
- packages/plugins/snap/package.json
- packages/plugins/sync/package.json
- packages/tempo/doc/4-advanced-reference/tempo.ticker.md
- packages/tempo/archive/tempo.api.md
🚧 Files skipped from review as they are similar to previous changes (34)
- packages/plugins/finance/tsconfig.json
- packages/plugins/batch/CHANGELOG.md
- packages/plugins/astro/test/tsconfig.json
- packages/plugins/sync/tsup.config.ts
- packages/tempo/doc/9-plugins/finance.md
- packages/plugins/finance/doc/index.md
- packages/plugins/sync/test/tsconfig.json
- packages/plugins/batch/test/tsconfig.json
- packages/plugins/snap/README.md
- packages/plugins/bin/tsconfig.json
- packages/plugins/finance/test/tsconfig.json
- packages/plugins/snap/test/tsconfig.json
- packages/plugins/batch/doc/index.md
- packages/plugins/snap/tsup.config.ts
- packages/plugins/batch/README.md
- packages/plugins/finance/README.md
- packages/plugins/finance/package.json
- packages/plugins/sync/README.md
- packages/plugins/sync/test/sync.test.ts
- packages/tempo/doc/9-plugins/snap.md
- packages/plugins/sync/src/AtomicClock.ts
- packages/plugins/snap/test/snap.test.ts
- packages/tempo/doc/9-plugins/batch.md
- packages/plugins/batch/src/worker.ts
- packages/plugins/astro/doc/index.md
- packages/plugins/astro/README.md
- packages/tempo/doc/9-plugins/sync.md
- packages/plugins/ticker/doc/index.md
- packages/plugins/snap/doc/index.md
- packages/plugins/batch/test/benchmark.ts
- packages/plugins/sync/doc/index.md
- packages/tempo/doc/9-plugins/astro.md
- packages/plugins/snap/src/index.ts
- packages/tempo/doc/9-plugins/ticker.md
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
Tempoera accessors and{era}/{eon}support.{h12}auto-meridiem formatting with:spaceanddotsmodifiers.