Feature/registry support#36
Closed
magmacomputing wants to merge 17 commits into
Closed
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 v3.0.0 consolidates Intl config into grouped keys ( ChangesTempo v3.0.0 Release
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/tempo/doc/tempo.config.md (1)
243-269:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the planner examples to the grouped
plannershape.The options table above documents
planner, but this section still tells readers to callTempo.init({ preFilter: true })andTempo.init({ layoutOrder: [...] }). Those snippets now teach the old API.Suggested doc fix
-Tempo.init({ preFilter: true }); +Tempo.init({ planner: { preFilter: true } });Tempo.init({ - layoutOrder: ['ymd', 'dt', 'tm', 'rel'] + planner: { layoutOrder: ['ymd', 'dt', 'tm', 'rel'] } });-**Observability**: Set `debug: true` along with `preFilter: true` to see a detailed "Planner summary" in the console, showing how many layouts were skipped for a given input. +**Observability**: Set `debug: true` along with `planner: { preFilter: true }` to see a detailed "Planner summary" in the console, showing how many layouts were skipped for a given 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/doc/tempo.config.md` around lines 243 - 269, The examples still show the old flat options (Tempo.init({ preFilter: ... }) and Tempo.init({ layoutOrder: ... })); update them to use the new grouped planner shape by replacing usages with the planner object (set planner.preFilter and planner.layoutOrder accordingly) and adjust the observability tip to mention enabling debug alongside planner.preFilter (e.g., debug: true + planner.preFilter) so the “Planner summary” logging remains accurate; search for Tempo.init examples and the mentions of preFilter/layoutOrder in this section and switch them to the planner.* form (referencing planner, preFilter, layoutOrder, and debug to locate the relevant snippets).packages/tempo/doc/tempo.cookbook.md (1)
227-284:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThese ticker recipes are broken in v3 as written.
Every example here calls
Tempo.ticker()from the core package, but v3 core now throws migration stubs unless@magmacomputing/tempo-plugin-tickeris installed and registered first. Readers copying any of these snippets will hit the stub immediately.Suggested doc fix
### Subscription Billing (Recurring Payments) Use a `seed` to anchor your subscription to a specific day, then use a month-based Ticker. ```typescript +import { Tempo } from '`@magmacomputing/tempo`'; +import { TickerModule } from '`@magmacomputing/tempo-plugin-ticker`'; + +Tempo.init({ license: 'YOUR_JWT_KEY' }); +Tempo.extend(TickerModule); + // Anchor to the 15th of the month await using billing = Tempo.ticker({ months: 1,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/doc/tempo.cookbook.md` around lines 227 - 284, The examples call Tempo.ticker() directly but v3 core throws migration stubs unless the ticker plugin is registered; before using Tempo.ticker (referenced in examples like the Subscription Billing, Fiscal Quarter Reporting, Daily Shift Management and Manual Sync snippets) initialize and extend Tempo with the ticker plugin by calling Tempo.init(...) with your license/JWT and then Tempo.extend(TickerModule) (TickerModule from `@magmacomputing/tempo-plugin-ticker`) so the ticker implementation is available to the examples.packages/tempo/src/module/module.duration.ts (1)
253-256:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
intl.durationFormatintoTempo.duration(...)too.The new config is threaded through
until()/since(), but the staticTempo.duration(...)path still only carrieslocaleandnumberFormat. That makesTempo.duration(input).format()ignore the globalintl.durationFormatsetting.Suggested fix
const ctx = { locale: this.config?.locale, - numberFormat: this.config?.intl?.numberFormat + numberFormat: this.config?.intl?.numberFormat, + durationFormat: this.config?.intl?.durationFormat };🤖 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.duration.ts` around lines 253 - 256, Tempo.duration(...) currently builds a ctx object in module.duration.ts that only includes locale and numberFormat so global intl.durationFormat is not applied; update the ctx construction used by Tempo.duration (the ctx variable in module.duration.ts) to also include this.config?.intl?.durationFormat (or equivalent key name used by Duration.format) so the returned duration instance inherits and uses the global durationFormat setting when .format() is called.package.json (1)
3-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign the root version with the v3 release metadata.
Line 3 still says
2.11.3, but Line 18 uses that root version as the source of truth forversion:sync. On this branch that would push@magmacomputing/tempoand@magmacomputing/libraryback down from3.0.0the next time the sync script runs.Suggested fix
- "version": "2.11.3", + "version": "3.0.0",🤖 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` around lines 3 - 18, The root package.json "version" field is still 2.11.3 while the release metadata expects v3; update the "version" value in package.json to match the v3 release (e.g., "3.0.0") so the version:sync script (node -e ... version:sync) does not downgrade `@magmacomputing/tempo` and `@magmacomputing/library` when it runs; ensure the top-level "version" string is changed and committed to keep the workspace sync consistent.packages/tempo/package.json (1)
205-210:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove
@magmacomputing/librarytodependencies.This package resolves
#libraryto@magmacomputing/libraryvia itsimportsmap, and the shipped Tempo sources use those imports at runtime. Leaving it indevDependenciesmeans npm consumers will install@magmacomputing/tempowithout its required runtime dependency.Suggested fix
"dependencies": { - "tslib": "^2.8.1" + "tslib": "^2.8.1", + "`@magmacomputing/library`": "3.0.0" }, "devDependencies": { "`@js-temporal/polyfill`": "^0.5.1", - "`@magmacomputing/library`": "3.0.0", "`@rollup/plugin-alias`": "^6.0.0",🤖 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/package.json` around lines 205 - 210, Move the runtime dependency "`@magmacomputing/library`" from devDependencies into dependencies in package.json so consumers install it; edit the "dependencies" object to include "`@magmacomputing/library`": "3.0.0" (removing it from "devDependencies") ensuring the package that resolves "`#library`" is present at runtime for the Tempo sources.
🤖 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/library/src/common/international.library.ts`:
- Around line 67-72: The function formatDuration currently swallows any
exceptions from getDF(...).format(...) and returns an empty string; change this
so formatting failures propagate instead of returning ''—either remove the
try/catch or rethrow the caught error (or return undefined) so callers can run
their fallback logic; locate formatDuration and getDF in
international.library.ts and ensure you do not hide native Intl.DurationFormat
errors by returning ''.
In `@packages/tempo/doc/migration-guide.md`:
- Around line 159-167: The migration guide text incorrectly uses pre-release
phrasing ("will be entirely removed in Tempo v3.0.0") even though this is the v3
guide; update the wording in the migration section that references the intl
config keys (`relativeTime` and `relativeTimeFormat`) and the example `new
Tempo({...})` so it states the current status (e.g., "removed in v3.0.0" or "no
longer supported in v3") instead of future-tense language, and ensure the
deprecation note clearly instructs users to migrate from `relativeTime` to
`relativeTimeFormat`.
- Around line 9-18: The migration note omits applying the license key; call
Tempo.init with the license JWT before registering the ticker plugin so the
plugin is activated (e.g., invoke Tempo.init({ license: 'YOUR_JWT_KEY' }) prior
to Tempo.extend(TickerModule)); update the example to show Tempo.init(...)
followed by Tempo.extend(TickerModule) and mention the license field name so
users know where to supply the key.
In `@packages/tempo/doc/tempo.duration.md`:
- Around line 56-60: The docs currently pass the Intl.RelativeTimeFormat
instance (autoFormat) to anchor.since, but Intl.RelativeTimeFormat is not
callable; update the example to pass the bound formatter function instead—use
autoFormat.format.bind(autoFormat) when calling anchor.since so the formatter
preserves its this and can be invoked by the library (reference symbols:
Intl.RelativeTimeFormat, autoFormat, anchor, anchor.since).
In `@packages/tempo/doc/tempo.format.md`:
- Around line 97-98: Update the week-of-year example values for the `{ww}` and
`{WW}` tokens: replace the current examples `17` and `17th` with the correct ISO
week number `43` and its ordinal `43rd` (for the date 2026-10-24) so `{ww}`
shows `43` and `{WW}` shows `43rd`.
In `@packages/tempo/src/module/module.duration.ts`:
- Around line 111-116: The code is incorrectly spreading duration-format options
(intlOpts) into formatNumber(), causing invalid NumberFormat keys like style:
'digital' to be passed; modify the call that currently spreads
...(ctx.numberFormat || {}), ...intlOpts into formatNumber() so that only
NumberFormat-safe options are forwarded (e.g., use ctx.numberFormat and/or a
filteredIntlNumberOpts that selects allowed NumberFormat keys such as
minimumFractionDigits, maximumFractionDigits, useGrouping) and do NOT spread
intlOpts into formatNumber(); locate the call site that uses formatNumber(..., {
style: 'unit', unit: unitName, unitDisplay: 'long', ...(ctx.numberFormat || {}),
...intlOpts }) and replace the spread of intlOpts with either nothing or a
filtered set of numeric-format options.
In `@packages/tempo/src/plugin/extend/extend.ticker.ts`:
- Around line 7-10: The declarations currently use overly permissive any for
Tempo.tickers and Tempo.ticker which hides the v3 breaking change; update the
declaration block so Tempo.tickers is typed appropriately (e.g., remove or
narrow any to the correct array type) and change the Tempo.ticker signature to
reflect that it no longer returns normal values (e.g., return type never or a
specific thrown-error type) or mark it as deprecated/throws so TypeScript
surfaces the breaking change at call sites; update the declare module block
where Tempo.tickers and function ticker(...args: any[]): any are defined to use
the new, explicit types instead of any.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 386-387: The current shallow merge (shape.config.intl = {
...shape.config.intl, ...discovery.intl }) loses nested keys; replace it with a
deep-merge so nested objects like relativeTimeFormat, durationFormat and
numberFormat are composed rather than overwritten. Locate the assignment that
touches discovery.intl and shape.config.intl in Tempo (tempo.class.ts) and use
an existing deep-merge utility (or add a small recursive merge helper) to merge
shape.config.intl with discovery.intl (e.g., deepMerge(shape.config.intl,
discovery.intl)), ensuring nested properties are merged instead of replaced.
Ensure the merge preserves existing keys when discovery.intl provides only
partial nested objects.
In `@packages/tempo/test/plugins/duration.balance.test.ts`:
- Around line 26-31: The test fails under different runtime implementations of
Intl.DurationFormat; update the assertion in the test for Tempo.duration
(symbol: Tempo.duration and method: format) to accept both possible outputs by
checking for native Intl.DurationFormat support (e.g., typeof Intl !==
'undefined' && 'DurationFormat' in Intl) and then asserting the expected string
accordingly, or simply allow either "1 yr, 5 days" or "1 year and 5 days" as
valid results for dur2.format({ locales: 'en-US' }) so the test passes under
both the native and polyfilled paths.
---
Outside diff comments:
In `@package.json`:
- Around line 3-18: The root package.json "version" field is still 2.11.3 while
the release metadata expects v3; update the "version" value in package.json to
match the v3 release (e.g., "3.0.0") so the version:sync script (node -e ...
version:sync) does not downgrade `@magmacomputing/tempo` and
`@magmacomputing/library` when it runs; ensure the top-level "version" string is
changed and committed to keep the workspace sync consistent.
In `@packages/tempo/doc/tempo.config.md`:
- Around line 243-269: The examples still show the old flat options
(Tempo.init({ preFilter: ... }) and Tempo.init({ layoutOrder: ... })); update
them to use the new grouped planner shape by replacing usages with the planner
object (set planner.preFilter and planner.layoutOrder accordingly) and adjust
the observability tip to mention enabling debug alongside planner.preFilter
(e.g., debug: true + planner.preFilter) so the “Planner summary” logging remains
accurate; search for Tempo.init examples and the mentions of
preFilter/layoutOrder in this section and switch them to the planner.* form
(referencing planner, preFilter, layoutOrder, and debug to locate the relevant
snippets).
In `@packages/tempo/doc/tempo.cookbook.md`:
- Around line 227-284: The examples call Tempo.ticker() directly but v3 core
throws migration stubs unless the ticker plugin is registered; before using
Tempo.ticker (referenced in examples like the Subscription Billing, Fiscal
Quarter Reporting, Daily Shift Management and Manual Sync snippets) initialize
and extend Tempo with the ticker plugin by calling Tempo.init(...) with your
license/JWT and then Tempo.extend(TickerModule) (TickerModule from
`@magmacomputing/tempo-plugin-ticker`) so the ticker implementation is available
to the examples.
In `@packages/tempo/package.json`:
- Around line 205-210: Move the runtime dependency "`@magmacomputing/library`"
from devDependencies into dependencies in package.json so consumers install it;
edit the "dependencies" object to include "`@magmacomputing/library`": "3.0.0"
(removing it from "devDependencies") ensuring the package that resolves
"`#library`" is present at runtime for the Tempo sources.
In `@packages/tempo/src/module/module.duration.ts`:
- Around line 253-256: Tempo.duration(...) currently builds a ctx object in
module.duration.ts that only includes locale and numberFormat so global
intl.durationFormat is not applied; update the ctx construction used by
Tempo.duration (the ctx variable in module.duration.ts) to also include
this.config?.intl?.durationFormat (or equivalent key name used by
Duration.format) so the returned duration instance inherits and uses the global
durationFormat setting when .format() is called.
🪄 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: 395d2416-d4ef-47cd-a52c-268704ade220
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
.gitignorepackage.jsonpackages/library/package.jsonpackages/library/src/common/international.library.tspackages/library/src/common/number.library.tspackages/tempo/CHANGELOG.mdpackages/tempo/doc/migration-guide.mdpackages/tempo/doc/release-notes-v3.0.0.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/doc/tempo.duration.mdpackages/tempo/doc/tempo.format.mdpackages/tempo/doc/tempo.month-day.mdpackages/tempo/doc/tempo.ticker.mdpackages/tempo/package.jsonpackages/tempo/src/library.index.tspackages/tempo/src/module/module.duration.tspackages/tempo/src/module/module.format.tspackages/tempo/src/plugin/extend/extend.ticker.tspackages/tempo/src/support/support.default.tspackages/tempo/src/support/support.enum.tspackages/tempo/src/support/support.init.tspackages/tempo/src/support/support.intl.tspackages/tempo/src/support/support.symbol.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tsconfig.jsonpackages/tempo/test/core/dispose.core.test.tspackages/tempo/test/instance/instance.since.rtf.test.tspackages/tempo/test/plugins/duration.balance.test.tspackages/tempo/test/plugins/slick.verification.test.tspackages/tempo/test/plugins/ticker.active.test.tspackages/tempo/test/plugins/ticker.hang.test.tspackages/tempo/test/plugins/ticker.options.test.tspackages/tempo/test/plugins/ticker.patterns.test.tspackages/tempo/test/plugins/ticker.pulse.test.tspackages/tempo/test/plugins/ticker.stop.test.tspackages/tempo/test/plugins/ticker.term.core.test.tspackages/tempo/test/plugins/ticker_cold_start.test.tspackages/tempo/test/support/error-handling.test.ts
💤 Files with no reviewable changes (13)
- packages/tempo/test/support/error-handling.test.ts
- packages/tempo/test/plugins/ticker.patterns.test.ts
- packages/tempo/test/plugins/ticker_cold_start.test.ts
- packages/tempo/test/plugins/ticker.hang.test.ts
- packages/tempo/test/plugins/ticker.pulse.test.ts
- packages/tempo/test/plugins/ticker.active.test.ts
- packages/tempo/test/plugins/ticker.options.test.ts
- packages/tempo/doc/tempo.ticker.md
- packages/tempo/test/plugins/ticker.term.core.test.ts
- packages/tempo/test/plugins/ticker.stop.test.ts
- packages/tempo/test/plugins/slick.verification.test.ts
- packages/tempo/src/support/support.init.ts
- packages/tempo/test/core/dispose.core.test.ts
Owner
Author
|
@coderabbitai please review |
Contributor
|
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/library/src/common/assertion.library.ts (1)
57-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
Temporalconstructors beforeinstanceof.
instanceofthrows aTypeErrorwhen the right-hand side isn’t a valid callable/constructor (e.g.,undefined). Here the predicate only checksglobalThis.Temporal, then unconditionally evaluatesobj instanceof globalThis.Temporal.<Ctor>for multiple constructors—so a partial polyfill/stub that omits one class makesisTemporal()throw instead of returningfalse. Add per-constructor guards (matching the patterns used by the specific helpers below).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/library/src/common/assertion.library.ts` around lines 57 - 65, The isTemporal predicate currently risks throwing TypeError because it uses (obj instanceof (globalThis as any).Temporal.SomeCtor) without first ensuring each Temporal constructor exists; update isTemporal to guard each instanceof check by verifying the specific constructor is defined (e.g., check (globalThis as any).Temporal.Instant before using instanceof) similar to patterns used by protoType/isDefined in the file, so the function returns false for partial polyfills/stubs instead of throwing; locate the isTemporal function and wrap each instanceof reference to Temporal.Instant, ZonedDateTime, PlainDate, PlainTime, PlainDateTime, Duration, PlainYearMonth, and PlainMonthDay with per-constructor existence checks.packages/tempo/doc/tempo.plugin.md (1)
28-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
defineExtensiondocs examples: missing closing brace forinstall()Both
defineExtensionexamples omit the}that closesinstall(TempoClass: any) { ... }, so the copied TypeScript is invalid. Add a}before});in both blocks.Suggested fix
export const MyPlugin = defineExtension({ name: 'MyPlugin', install(TempoClass: any) { // 1. Add a static method TempoClass.myStaticMethod = () => { /* ... */ }; // 2. Add an instance method (on the prototype) TempoClass.prototype.toHoliday = function() { return factory(this.add({ days: 1 })); }; + } });export const MyPlugin = defineExtension({ name: 'MyPlugin', install(TempoClass: any) { TempoClass.myTool = function(arg1: any): MyPluginTypes.Instance { const instance = new MyPluginInstance(arg1); const proxy = new Proxy((() => instance.doSomething()) as any, { get: (_, prop) => { // Map proxy properties to instance methods if (prop in instance) return (instance as any)[prop].bind(instance); return (instance as any)[prop]; }, apply: (target) => target() }) as unknown as MyPluginTypes.Instance; return instance.bootstrap(proxy); }; + } });🤖 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.plugin.md` around lines 28 - 38, The examples for defineExtension (e.g., MyPlugin) are missing the closing brace for the install method; update the example(s) so the install(TempoClass: any) { ... } block is properly closed by adding a '}' before the final '});' so that the install function (which sets TempoClass.myStaticMethod and TempoClass.prototype.toHoliday) is syntactically complete.packages/tempo/index.md (1)
98-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the DEV registry override on the failure path too.
If the import at Line 106 rejects,
registry.hasnever gets put back and the HMR serializer registry stays permanently stubbed for the rest of the session. Move the restoration into afinallyso the workaround is always unwound.Proposed fix
let initPromise = (async () => { + let registry, originalHas try { // HMR Safeguard for development only (stripped in production) - let registry, originalHas if (import.meta.env.DEV) { const registryKey = Symbol.for('$LibrarySerializerRegistry') registry = globalThis[registryKey] ??= new Map() // HMR Workaround: Temporarily bypass registry presence checks to allow class re-hydration originalHas = registry.has.bind(registry) registry.has = () => false } const { Tempo } = await import('`@magmacomputing/tempo`') - - if (import.meta.env.DEV) registry.has = originalHas Tempo.init() return Tempo } catch (e) { console.error('Tempo failed to initialize:', e) initFailed = true initPromise = undefined throw e + } finally { + if (import.meta.env.DEV && registry && originalHas) { + registry.has = originalHas + } } })()🤖 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/index.md` around lines 98 - 108, The DEV-time workaround sets registry.has = () => false but only restores it after a successful dynamic import, so if the import of '`@magmacomputing/tempo`' rejects the stub remains; wrap the dynamic import and any use of Tempo in a try/finally (or ensure finally runs) so that registry.has is always reset to originalHas, referencing the registry variable, originalHas, the Symbol.for('$LibrarySerializerRegistry') lookup, and the dynamic import('`@magmacomputing/tempo`') call; keep the stub assignment in the DEV branch but move the restoration into the finally block so the registry is unwound on both success and failure.
♻️ Duplicate comments (1)
packages/tempo/src/plugin/extend/extend.ticker.ts (1)
10-11:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the deprecated
Tempo.tickerstub non-callable in TypeScript.
function ticker(...args: any[]): neverstill accepts every old call shape, so this v3 break is largely invisible until runtime. Narrow the declaration so the call expression itself fails to type-check, and mark it deprecated on the augmented API.#!/bin/bash set -euo pipefail printf '\nTicker declaration surface:\n' rg -n -C2 'const tickers:|function ticker\(' packages/tempo/src/plugin/extend/extend.ticker.ts printf '\nPotential ticker call sites in the repo:\n' rg -nP --type=ts --type=tsx '\.ticker\s*\('Expected result: the declaration still shows
...args: any[], and any remaining*.ticker(...)call sites will continue to satisfy that signature even though every runtime path now throws.🤖 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/plugin/extend/extend.ticker.ts` around lines 10 - 11, Replace the callable stub with a non-callable constant and mark it deprecated: change the function declaration "function ticker(...args: any[]): never" to a non-callable "const ticker: never" (keep "const tickers: never[]" as-is) and add a JSDoc `@deprecated` comment above the ticker declaration so TypeScript will error on any remaining ticker(...) call sites and the API is clearly marked deprecated; target the symbols "tickers" and "ticker" in extend.ticker.ts when making this change.
🧹 Nitpick comments (3)
packages/tempo/doc/sandbox-factory.md (1)
69-69: ⚡ Quick winPrefer the string log level in the docs.
debug: 5is tied to the current numeric value ofLOG.Trace.debug: 'trace'expresses the same intent through the public API and is less likely to drift in future enum reorderings.🤖 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/sandbox-factory.md` at line 69, Replace the numeric log level "debug: 5" in the "Use Debug Mode" example with the string form "debug: 'trace'"; this keeps the intent equal to LOG.Trace but uses the public API (string log level) to avoid future enum reordering issues—update the example text that currently reads "debug: 5" to "debug: 'trace'".packages/tempo/vitest.config.ts (1)
17-20: ⚡ Quick winMake the premium
tsconfig.jsonprobe depth-agnostic (current repo layout matches, external premium layout may not).
- In
packages/tempo/vitest.config.ts,resolve(dirname(licensePremium), '../tsconfig.json')resolves topackages/tempo/src/tsconfig.jsonfor the only in-reposupport.license.ts(packages/tempo/src/support/support.license.ts), so the alias logic works for the default layout.- The check still hardcodes a fixed relative depth from
support.license.ts. If a Premium build shipssupport.license.tsat a different depth,isPremiumAvailablewill be false and#tempo/licensewill silently fall back tolicenseDefault.- Relax the probe to walk upward from
dirname(licensePremium)to the nearesttsconfig.json(and apply the same change inpackages/tempo/rollup.config.jsto keep behavior consistent).🤖 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/vitest.config.ts` around lines 17 - 20, The current isPremiumAvailable check uses a fixed relative path resolve(dirname(licensePremium), '../tsconfig.json') which breaks if support.license.ts is at a different depth; change the probe to walk upward from dirname(licensePremium) looking for the nearest tsconfig.json (stop at filesystem root) and use that found path in the existence check (so isPremiumAvailable checks Boolean(licensePremium && fs.existsSync(licensePremium) && fs.existsSync(foundTsconfigPath))). Apply the identical upward-search logic where the same relative resolve is used in packages/tempo/rollup.config.js so both vitest.config.ts and rollup.config.js consistently locate the nearest tsconfig.json instead of assuming a fixed ../tsconfig.json.packages/tempo/test/core/alias-engine-protochain.test.ts (1)
10-23: ⚡ Quick winRecreate the alias chain in
beforeEach.These engines are instantiated once for the whole suite, but Line 47 mutates
localShape.aliasEngine. That makes the tests order-dependent and can leak aliases into retries or future cases. BuildglobalShape/sandboxShape/localShapeper test 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/tempo/test/core/alias-engine-protochain.test.ts` around lines 10 - 23, Tests currently reuse a single shared AliasEngine chain (globalShape, sandboxShape, localShape) and mutate localShape.aliasEngine during tests, causing order-dependent leaks; move creation of globalShape, sandboxShape, and localShape into a beforeEach so each test gets a fresh chain: instantiate new AliasEngine() for globalShape, create sandboxShape via Object.create(globalShape) with new AliasEngine({ parent: globalShape.aliasEngine }), and likewise create localShape as Object.create(sandboxShape) with new AliasEngine({ parent: sandboxShape.aliasEngine }); registerAliases('evt', ...) on each new instance inside beforeEach to avoid cross-test state leakage.
🤖 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/library/doc/browser/types.md`:
- Line 29: Update the documentation for the debug property to describe the
numeric verbosity scale instead of just its type: state that debug is a
non-negative integer where 0 = no debug output, larger values increase verbosity
(e.g., 1 = errors, 2 = warnings, 3 = info, 4 = verbose, 5 = trace), indicate the
supported/expected range (suggest 0–5) and the default when omitted (e.g.,
default 0), and add a short example showing usage with 0 and 5; reference the
`debug` field in the types.md entry so readers migrating from the boolean
understand the mapping.
In `@packages/library/src/browser/mapper.library.ts`:
- Around line 27-31: The defaults object currently enables verbose logging by
setting debug: 3 which makes the mapper log sensitive location/reverse-geocode
data by default; change the defaults constant (defaults as MapOpts) to set
debug: 0 (or remove the debug field) so logging is opt-in, and ensure any use of
the Logger instance (log) and branches that check opts.debug continue to gate
output on a positive debug value (e.g., in functions that reference defaults,
MapOpts, or the log variable such as mapStore initialization and any map/mapper
functions).
In `@packages/library/src/common/boundary.library.ts`:
- Around line 11-14: The docstring for the silent property is out of sync with
raise(): update the comment for silent (the property in the boundary type) to
state that setting silent suppresses telemetry/logging for raise() regardless of
whether the error is rethrown or swallowed, or alternatively change the behavior
inside raise() (the raise function) to only suppress telemetry when catch is
true; pick one approach and make the docs and implementation consistent by
referencing the silent property and the raise() function so callers understand
the exact suppression semantics.
In `@packages/library/src/common/cipher.class.ts`:
- Around line 16-18: The module currently creates a single module-scoped IV
(_vector) and reuses it for every AES-GCM operation; change this so a new random
IV is generated per encryption call (inside the Cipher.encrypt method) using
crypto.getRandomValues(new Uint8Array(12 or 16)) and encode/attach that IV to
the returned ciphertext (e.g., prefix or include in a container) and then have
Cipher.decrypt extract and use that IV for decryption; update any other places
that rely on the module _vector (including the symmetric key generation areas
around _cryptoKey/_asymmetricKey and the code referenced near the
Cipher.encrypt/Cipher.decrypt implementations) to remove reuse of _vector and
ensure decrypt reads the per-message IV from the payload.
In `@packages/library/src/common/class.library.ts`:
- Around line 15-17: getClassName currently falls back to String(contextName)
which yields the literal "undefined" for anonymous decorator contexts; change
getClassName<T extends Constructor>(value, contextName) to return
getSafeTag(value) ?? (contextName === undefined ? undefined :
String(contextName)) (or alternatively fall back to value.name || undefined) so
an actual undefined is propagated instead of the string "undefined"; update
callers if necessary to handle undefined keys (references: getClassName,
registerType, registerSerializable, and the `@Static` wrapper TypeError message
generation).
In `@packages/library/src/common/logger.class.ts`:
- Around line 54-56: In Logger.#emit(), avoid directly accessing
msg[0][sym.$LogConfig] which can throw for Proxies or throwing getters; wrap the
detection in a try/catch (or use a safe accessor) when reading the $LogConfig
symbol from the first argument, e.g. attempt to read the symbol into a local
variable inside the try block and only call msg.shift() to set config if the
read succeeds; ensure the rest of `#emit`() uses the same config variable so the
risky access is contained to detection and cannot throw later.
In `@packages/library/src/common/pledge.class.ts`:
- Around line 63-65: The static getter Pledge.status currently casts _static to
Pledge.Status<typeof Pledge> but _static lacks the required state field; update
the getter to return an object that preserves _static properties and supplies
the missing state (e.g. return { ..._static, state: Pledge.state } as
Pledge.Status<typeof Pledge>) so callers of Pledge.status.state or Pledge.init()
receive the actual symbol; modify the Pledge.status getter accordingly to
construct and return the complete Status shape.
In `@packages/library/src/common/type.library.ts`:
- Around line 205-209: registerType currently picks getSafeTag(cls) before the
explicit caller-supplied type, causing user-provided aliases to be ignored;
change the selection so the provided type parameter wins (i.e., prefer type,
then tag, then cls.name) when computing the name used for registration (refer to
the tag and name variables and the registerType function) so explicit aliases
like the `_original` registrations are preserved.
In `@packages/tempo/.vitepress/config.ts`:
- Around line 52-56: The sidebar item uses a broken link '/doc/tempo.term';
update the items array entry that currently has { text: 'Terms Plugins', link:
'/doc/tempo.term' } to point to the existing doc (e.g. '/doc/tempo.plugin') or
add the missing file 'tempo.term.md' to the docs—modify the link string in the
items array (the object with text 'Terms Plugins') so it matches an existing
document like '/doc/tempo.plugin'.
In `@packages/tempo/doc/architecture.md`:
- Around line 30-33: The documentation wrongly shows using
Symbol.for($LogConfig); update the text to reflect the actual API: explain that
$LogConfig is already a Symbol created via Symbol.for('$LibraryLogConfig') and
that diagnostic metadata should be attached using the symbol variable directly
(e.g., config[$LogConfig] or config[sym.$LogConfig]) rather than calling
Symbol.for on $LogConfig; update the three mentions ("Symbol-Gated" bullet and
any examples) to reference the direct symbol usage and the canonical creation
Symbol.for('$LibraryLogConfig').
In `@packages/tempo/doc/tempo.extension.md`:
- Line 71: Update the explanatory text to use the correct Temporal type: change
the reference to "raw Temporal.PlainDateTime" to "Temporal.ZonedDateTime" and
clarify that .toDateTime() returns a zone-aware Temporal.ZonedDateTime (so
plugin authors understand they still have zone information when accessing
properties like dayOfWeek/dayOfYear/daysInMonth). Ensure the phrasing around
.toDateTime() and examples mention Temporal.ZonedDateTime instead of
PlainDateTime (referencing .toDateTime() and the Temporal.ZonedDateTime type).
In `@packages/tempo/src/engine/engine.lexer.ts`:
- Around line 224-225: The log line in the invalid-date branch currently embeds
the word "config" into the message and omits the actual config object, which
prevents honoring config.catch/config.silent; update the call to logError in the
branch that checks Number.isFinite(year/month/day) so it passes the real config
object as the second argument (e.g., logError(message, config)) rather than
baking "config" into the string, and keep a clear message including
year/month/day to aid debugging; locate the call to logError near the
Number.isFinite check in engine.lexer.ts and make this change so downstream
fallback to dateTime behaves correctly.
In `@packages/tempo/src/module/module.mutate.ts`:
- Line 63: The logError calls inside the mutation engine are interpolating
this.config into the message string instead of passing the actual config object
as the second argument; update both occurrences (the one logging "Infinite
recursion detected in mutation engine for key: ${key}, this.config, adjust:
${adjust}, depth: ${state.mutateDepth}" and the similar call around the later
occurrence) to call logError(message, this.config) so the logger receives the
real config object as the second parameter; locate calls to logError in
module.mutate.ts (the mutation engine functions) and change their signatures
accordingly.
In `@packages/tempo/src/module/module.parse.ts`:
- Around line 307-324: The new trace/debug calls eagerly build expensive
payloads (orderedPatterns.map(...), JSON.stringify(groups)) on the hot parsing
path; modify the calls around logTrace/logDebug in module.parse (including the
loop that calls _ParseEngine.parseMatch and the composer trace call) to guard
construction with a log level check or pass a lazy callback to the logger so the
map/stringify runs only when tracing/debugging is enabled—use the existing
logger level predicate (or add one) before computing orderedPatterns.map(...)
and before JSON.stringify(groups) to avoid unnecessary work when diagnostics are
off.
In `@packages/tempo/src/support/support.default.ts`:
- Line 195: The default logging level was changed from false to LOG.Info, which
opts all consumers into console logging; revert the default for the exported
"debug" setting in support.default.ts from LOG.Info back to false (i.e., keep
console logging opt-in) by updating the "debug" property in the defaults object,
and run any affected tests or type checks to ensure no callers expect a
non-boolean value.
In `@packages/tempo/src/support/support.init.ts`:
- Around line 307-309: The log call inside the Tempo#extend validation branch is
malformed: replace the current logError invocation so the message string does
not contain the literal text "state.config" and instead pass state.config as the
optional config/metadata argument to logError; construct a clear message using
String(unit ?? arg.value) and the expected units ('ss','ms','us','ns') and call
logError(message, state.config) so logging behavior respects the config and the
message is readable; update references in this branch around isUndefined, unit,
logError, arg.value accordingly.
In `@packages/tempo/src/support/support.symbol.ts`:
- Line 8: The current isTempo type guard uses isDefined(tempo?.[sym.$Identity])
which can treat falsy sentinel values (e.g., false) as defined and incorrectly
allow non-Tempo objects; update the isTempo guard (exported function isTempo) to
perform a strict comparison against the sentinel boolean (tempo?.[sym.$Identity]
=== true) so only objects with the actual Tempo sentinel pass the guard,
preserving the TempoBrand invariant.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 701-703: _reset _lifecycle.ready before starting the re-init
sequence so other code doesn't see the library as ready while state is being
rebuilt: in Tempo.init (around the _lifecycle.initialising check and where you
set _lifecycle.initialising = true) set _lifecycle.ready = false immediately
before any global state reset/rebuild logic; keep _lifecycle.initialising
semantics the same and ensure _lifecycle.ready is only set true again after all
rebuild steps complete (and after you clear _lifecycle.initialising).
- Around line 241-244: In the catch block where the code constructs Intl.Locale
(inside Tempo._locale usage in Tempo class / the `#isMonthDay` path), change the
logWarn call to match the new signature logWarn(msg, config, ...extra): replace
the current logWarn(e, `Invalid locale encountered in `#isMonthDay`: ${locale}.
Falling back to en-US.`, shape.config) with logWarn(`Invalid locale encountered
in `#isMonthDay`: ${locale}. Falling back to en-US.`, shape.config, e) so the
warning message and shape.config are passed correctly and the original error is
preserved as extra context.
---
Outside diff comments:
In `@packages/library/src/common/assertion.library.ts`:
- Around line 57-65: The isTemporal predicate currently risks throwing TypeError
because it uses (obj instanceof (globalThis as any).Temporal.SomeCtor) without
first ensuring each Temporal constructor exists; update isTemporal to guard each
instanceof check by verifying the specific constructor is defined (e.g., check
(globalThis as any).Temporal.Instant before using instanceof) similar to
patterns used by protoType/isDefined in the file, so the function returns false
for partial polyfills/stubs instead of throwing; locate the isTemporal function
and wrap each instanceof reference to Temporal.Instant, ZonedDateTime,
PlainDate, PlainTime, PlainDateTime, Duration, PlainYearMonth, and PlainMonthDay
with per-constructor existence checks.
In `@packages/tempo/doc/tempo.plugin.md`:
- Around line 28-38: The examples for defineExtension (e.g., MyPlugin) are
missing the closing brace for the install method; update the example(s) so the
install(TempoClass: any) { ... } block is properly closed by adding a '}' before
the final '});' so that the install function (which sets
TempoClass.myStaticMethod and TempoClass.prototype.toHoliday) is syntactically
complete.
In `@packages/tempo/index.md`:
- Around line 98-108: The DEV-time workaround sets registry.has = () => false
but only restores it after a successful dynamic import, so if the import of
'`@magmacomputing/tempo`' rejects the stub remains; wrap the dynamic import and
any use of Tempo in a try/finally (or ensure finally runs) so that registry.has
is always reset to originalHas, referencing the registry variable, originalHas,
the Symbol.for('$LibrarySerializerRegistry') lookup, and the dynamic
import('`@magmacomputing/tempo`') call; keep the stub assignment in the DEV branch
but move the restoration into the finally block so the registry is unwound on
both success and failure.
---
Duplicate comments:
In `@packages/tempo/src/plugin/extend/extend.ticker.ts`:
- Around line 10-11: Replace the callable stub with a non-callable constant and
mark it deprecated: change the function declaration "function ticker(...args:
any[]): never" to a non-callable "const ticker: never" (keep "const tickers:
never[]" as-is) and add a JSDoc `@deprecated` comment above the ticker declaration
so TypeScript will error on any remaining ticker(...) call sites and the API is
clearly marked deprecated; target the symbols "tickers" and "ticker" in
extend.ticker.ts when making this change.
---
Nitpick comments:
In `@packages/tempo/doc/sandbox-factory.md`:
- Line 69: Replace the numeric log level "debug: 5" in the "Use Debug Mode"
example with the string form "debug: 'trace'"; this keeps the intent equal to
LOG.Trace but uses the public API (string log level) to avoid future enum
reordering issues—update the example text that currently reads "debug: 5" to
"debug: 'trace'".
In `@packages/tempo/test/core/alias-engine-protochain.test.ts`:
- Around line 10-23: Tests currently reuse a single shared AliasEngine chain
(globalShape, sandboxShape, localShape) and mutate localShape.aliasEngine during
tests, causing order-dependent leaks; move creation of globalShape,
sandboxShape, and localShape into a beforeEach so each test gets a fresh chain:
instantiate new AliasEngine() for globalShape, create sandboxShape via
Object.create(globalShape) with new AliasEngine({ parent:
globalShape.aliasEngine }), and likewise create localShape as
Object.create(sandboxShape) with new AliasEngine({ parent:
sandboxShape.aliasEngine }); registerAliases('evt', ...) on each new instance
inside beforeEach to avoid cross-test state leakage.
In `@packages/tempo/vitest.config.ts`:
- Around line 17-20: The current isPremiumAvailable check uses a fixed relative
path resolve(dirname(licensePremium), '../tsconfig.json') which breaks if
support.license.ts is at a different depth; change the probe to walk upward from
dirname(licensePremium) looking for the nearest tsconfig.json (stop at
filesystem root) and use that found path in the existence check (so
isPremiumAvailable checks Boolean(licensePremium &&
fs.existsSync(licensePremium) && fs.existsSync(foundTsconfigPath))). Apply the
identical upward-search logic where the same relative resolve is used in
packages/tempo/rollup.config.js so both vitest.config.ts and rollup.config.js
consistently locate the nearest tsconfig.json instead of assuming a fixed
../tsconfig.json.
🪄 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: 572af541-f4ba-4ea6-b8c6-86d428e8766d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (72)
CHANGELOG.mdpackage.jsonpackages/library/doc/browser/types.mdpackages/library/src/browser/mapper.library.tspackages/library/src/browser/webstore.class.tspackages/library/src/common.index.tspackages/library/src/common/assertion.library.tspackages/library/src/common/boundary.library.tspackages/library/src/common/cipher.class.tspackages/library/src/common/class.library.tspackages/library/src/common/logger.class.tspackages/library/src/common/logify.class.tspackages/library/src/common/pledge.class.tspackages/library/src/common/symbol.library.tspackages/library/src/common/type.library.tspackages/library/src/server/file.library.tspackages/library/test/common/boundary.library.test.tspackages/library/test/common/logger.class.test.tspackages/library/test/common/logify.class.test.tspackages/library/vitest.config.tspackages/tempo/.vitepress/config.tspackages/tempo/CHANGELOG.mdpackages/tempo/bench/bench.parse.prefilter.e2e.tspackages/tempo/doc/architecture.mdpackages/tempo/doc/migration-guide.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/doc/sandbox-factory.mdpackages/tempo/doc/soft_freeze_strategy.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.debugging.mdpackages/tempo/doc/tempo.extension.mdpackages/tempo/doc/tempo.pledge.mdpackages/tempo/doc/tempo.plugin.mdpackages/tempo/doc/tempo.ticker.mdpackages/tempo/index.mdpackages/tempo/package.jsonpackages/tempo/rollup.config.jspackages/tempo/src/engine/engine.alias.tspackages/tempo/src/engine/engine.composer.tspackages/tempo/src/engine/engine.guard.tspackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/engine/engine.normalizer.tspackages/tempo/src/engine/engine.pattern.tspackages/tempo/src/engine/engine.planner.tspackages/tempo/src/engine/engine.term.tspackages/tempo/src/module/module.duration.tspackages/tempo/src/module/module.mutate.tspackages/tempo/src/module/module.parse.tspackages/tempo/src/plugin/extend/extend.ticker.tspackages/tempo/src/plugin/plugin.util.tspackages/tempo/src/support/support.default.tspackages/tempo/src/support/support.error.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/support.init.tspackages/tempo/src/support/support.license.tspackages/tempo/src/support/support.register.tspackages/tempo/src/support/support.symbol.tspackages/tempo/src/support/support.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/test/core/alias-engine-protochain.test.tspackages/tempo/test/core/alias-engine.test.tspackages/tempo/test/core/dispose.core.test.tspackages/tempo/test/engine/parse.prefilter.flag.test.tspackages/tempo/test/issues/issue-fixes.test.tspackages/tempo/test/plugins/licensing.full.test.tspackages/tempo/test/support/library-import.test.tspackages/tempo/test/support/symbol-import.test.tspackages/tempo/vitest.config.tspackages/tempo/vitest.workspace.tsvitest.config.tsvitest.workspace.ts
💤 Files with no reviewable changes (5)
- vitest.workspace.ts
- packages/library/test/common/logify.class.test.ts
- packages/library/src/common/logify.class.ts
- packages/tempo/vitest.workspace.ts
- packages/tempo/test/support/library-import.test.ts
✅ Files skipped from review due to trivial changes (9)
- packages/tempo/doc/soft_freeze_strategy.md
- packages/tempo/doc/tempo.pledge.md
- packages/tempo/src/support/support.error.ts
- packages/tempo/rollup.config.js
- packages/tempo/doc/releases/v2.x.md
- CHANGELOG.md
- packages/tempo/doc/tempo.debugging.md
- packages/tempo/CHANGELOG.md
- packages/tempo/doc/migration-guide.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/tempo/test/core/dispose.core.test.ts
- packages/tempo/src/module/module.duration.ts
- packages/tempo/package.json
- packages/tempo/src/tempo.type.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/library/src/common/cipher.class.ts`:
- Around line 75-91: encrypt() currently converts IV+ciphertext into UTF-8 via
Cipher.decodeBuffer (lossy) while decrypt() expects binary, breaking round-trip;
fix by returning a deterministic binary or text-safe representation from
Cipher.encrypt (e.g., return the raw Uint8Array/ArrayBuffer of combined or
base64-encode the combined bytes) and update Cipher.decrypt to accept and decode
exactly that same format (if using binary, accept ArrayBuffer/Uint8Array and
skip Cipher.decodeBuffer; if using base64, decode base64 to a Uint8Array before
slicing IV and ciphertext). Ensure you update references to Cipher.encodeBuffer
/ Cipher.decodeBuffer usage so encrypt produces the chosen format and decrypt
reverses it exactly.
In `@packages/library/src/common/class.library.ts`:
- Around line 150-160: The declarations inside the switch case 'class' (safeName
and wrapper) must be block-scoped to satisfy noSwitchDeclarations; wrap the
entire case 'class' body in braces so const safeName and const wrapper are
declared inside a local block. Locate the switch handling for kind and modify
the case 'class' block (around safeName, wrapper and the class expression that
throws the TypeError) to be enclosed in { ... } so those consts are scoped to
the case only.
In `@packages/tempo/bin/push-docs.sh`:
- Around line 25-26: Replace the unconstrained pulls with fast-forward-only
pulls so local main cannot be merged in unexpectedly: change the two occurrences
of the git pull command (the ones currently invoking "git pull origin main" in
the push-docs.sh script) to use a fast-forward-only strategy (e.g., "git pull
--ff-only origin main") so the script fails instead of creating a merge and
pushing unintended commits.
- Around line 5-7: The DOC_PATHS variable in push-docs.sh is relative to the
current working directory and breaks when callers run the script from the repo
root; change the script to compute an anchored repo root (e.g. REPO_ROOT="$(git
rev-parse --show-toplevel)") and redefine DOC_PATHS to use absolute/anchored
paths built from that REPO_ROOT (for example point to
"$REPO_ROOT/packages/tempo/doc", "$REPO_ROOT/packages/tempo/img", etc.), and
ensure all later uses of DOC_PATHS in push-docs.sh (checkout/copy/publish steps)
reference these anchored paths so the script behaves the same regardless of
where it was invoked from.
In `@packages/tempo/rollup.config.js`:
- Around line 17-31: The rollup logic currently treats a missing ancestor
tsconfig.json as non-premium which lets builds silently fall back; update the
premium-detection in rollup.config.js to fail closed by checking for the case
where licensePremium (TEMPO_LICENSE_PATH) exists but foundTsconfigPath is empty
and throw a clear error instead of proceeding—use the existing symbols
foundTsconfigPath and licensePremium (and the computed isPremiumAvailable) to
detect this mismatch and throw an Error explaining that TEMPO_LICENSE_PATH is
present but no ancestor tsconfig.json was found, or alternatively align the
package publish gate to use the same isPremiumAvailable predicate so behavior
matches.
🪄 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: fc06de57-10cd-48a8-82b3-a73ee36af6d3
📒 Files selected for processing (25)
package.jsonpackages/library/doc/browser/types.mdpackages/library/src/browser/mapper.library.tspackages/library/src/common/boundary.library.tspackages/library/src/common/cipher.class.tspackages/library/src/common/class.library.tspackages/library/src/common/logger.class.tspackages/library/src/common/pledge.class.tspackages/library/src/common/type.library.tspackages/tempo/.vitepress/config.tspackages/tempo/bin/push-docs.shpackages/tempo/doc/architecture.mdpackages/tempo/doc/sandbox-factory.mdpackages/tempo/doc/tempo.extension.mdpackages/tempo/package.jsonpackages/tempo/rollup.config.jspackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/module/module.mutate.tspackages/tempo/src/module/module.parse.tspackages/tempo/src/plugin/extend/extend.ticker.tspackages/tempo/src/support/support.init.tspackages/tempo/src/support/support.symbol.tspackages/tempo/src/tempo.class.tspackages/tempo/test/core/alias-engine-protochain.test.tspackages/tempo/vitest.config.ts
✅ Files skipped from review due to trivial changes (1)
- packages/tempo/doc/tempo.extension.md
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/tempo/doc/architecture.md
- packages/library/src/common/boundary.library.ts
- packages/tempo/package.json
- package.json
- packages/tempo/src/plugin/extend/extend.ticker.ts
- packages/library/src/common/pledge.class.ts
- packages/tempo/.vitepress/config.ts
- packages/library/src/common/type.library.ts
- packages/tempo/vitest.config.ts
- packages/library/src/browser/mapper.library.ts
- packages/tempo/src/engine/engine.lexer.ts
- packages/tempo/src/module/module.mutate.ts
- packages/tempo/src/support/support.init.ts
- packages/tempo/src/support/support.symbol.ts
- packages/tempo/src/module/module.parse.ts
- packages/tempo/src/tempo.class.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/library/src/common/cipher.class.ts (1)
75-91:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winStop round-tripping plaintext through
Uint16ArrayinCipher.encodeBuffer/decodeBuffer.
Cipher.encodeBuffer()turns UTF-8 bytes into aUint16Array, andCipher.decrypt()wraps the decrypted plaintext bytes back intoUint16ArraybeforeTextDecoder—causing embedded NULs in the output (e.g.,"AB"→"A\0B\0").🔧 Localized fix
-static encodeBuffer = (str: string) => new Uint16Array(new TextEncoder().encode(str)); -static decodeBuffer = (buf: Uint16Array) => new TextDecoder(keys.Encoding).decode(buf); +static encodeBuffer = (str: string) => new TextEncoder().encode(str); +static decodeBuffer = (buf: ArrayBuffer | ArrayBufferView) => + new TextDecoder(keys.Encoding).decode( + buf instanceof ArrayBuffer + ? new Uint8Array(buf) + : new Uint8Array(buf.buffer, buf.byteOffset, buf.byteLength) + ); static encrypt = async (data: any) => { const iv = crypto.getRandomValues(new Uint8Array(16)); const cipherBuf = await subtle.encrypt({ name: keys.TypeKey, iv }, await _cryptoKey, Cipher.encodeBuffer(data)); @@ return subtle.decrypt({ name: keys.TypeKey, iv }, await _cryptoKey, data) - .then(result => new Uint16Array(result)) - .then(Cipher.decodeBuffer); + .then(Cipher.decodeBuffer); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/library/src/common/cipher.class.ts` around lines 75 - 91, The decrypt path is round-tripping plaintext through Uint16Array causing embedded NULs; stop using Uint16Array and use Uint8Array/ArrayBuffer for raw UTF-8 bytes. Update Cipher.encodeBuffer and Cipher.decodeBuffer to operate on Uint8Array (UTF-8 bytes) instead of Uint16Array, and modify Cipher.decrypt to convert the decrypted ArrayBuffer to a Uint8Array (not new Uint16Array(result)) before calling Cipher.decodeBuffer; ensure subtle.encrypt/subtle.decrypt are passed ArrayBuffers/Uint8Arrays consistently.
🤖 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/library/src/common/cipher.class.ts`:
- Around line 75-91: The decrypt path is round-tripping plaintext through
Uint16Array causing embedded NULs; stop using Uint16Array and use
Uint8Array/ArrayBuffer for raw UTF-8 bytes. Update Cipher.encodeBuffer and
Cipher.decodeBuffer to operate on Uint8Array (UTF-8 bytes) instead of
Uint16Array, and modify Cipher.decrypt to convert the decrypted ArrayBuffer to a
Uint8Array (not new Uint16Array(result)) before calling Cipher.decodeBuffer;
ensure subtle.encrypt/subtle.decrypt are passed ArrayBuffers/Uint8Arrays
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65eaa74b-eab1-4d6e-8ec5-3e38f8f5104c
📒 Files selected for processing (9)
packages/library/src/common/cipher.class.tspackages/library/src/common/class.library.tspackages/tempo/bin/push-docs.shpackages/tempo/doc/commercial.mdpackages/tempo/doc/tempo.extension.mdpackages/tempo/doc/tempo.layout.mdpackages/tempo/doc/tempo.plugin.mdpackages/tempo/doc/tempo.term.mdpackages/tempo/rollup.config.js
✅ Files skipped from review due to trivial changes (4)
- packages/tempo/doc/commercial.md
- packages/tempo/doc/tempo.layout.md
- packages/tempo/doc/tempo.plugin.md
- packages/tempo/doc/tempo.extension.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/tempo/rollup.config.js
- packages/library/src/common/class.library.ts
- packages/tempo/bin/push-docs.sh
magmacomputing
force-pushed
the
feature/registry-support
branch
from
5be28ba to
bd7ddc0
Compare
Owner
Author
|
abandon this branch, as it has diverge too greatly from main |
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.
Version 3.0.0 is a standardization release
Summary by CodeRabbit
Breaking Changes
relativeTime→relativeTimeFormat;debugnow accepts numeric or string levels.New Features
Improvements
Documentation
Tests