Release/v3.0.x#38
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughScopes plugin/term registries per Internal.State, extracts per-state license management and validation, makes Tempo sandbox-aware (licenses/plugins per-state), stamps TEMPO_VERSION=3.0.1, and updates build, test, import-map, docs, and CI wiring for the new license/plugin layout. ChangesPlugin Database Refactoring and Release
Sequence DiagramsequenceDiagram
participant App as App
participant Tempo as Tempo.extend/create
participant LicenseMgr as plugin/license/license.manager
participant State as Internal.State
participant Validator as `#tempo/license`
App->>Tempo: provide license key
Tempo->>LicenseMgr: setLicense(State, key)
LicenseMgr->>State: set Pending + install jws Pledge
Tempo->>LicenseMgr: validateLicenseState(license, jws)
LicenseMgr->>Validator: verify(license.key)
Validator-->>LicenseMgr: verification result
LicenseMgr-->>State: resolve/reject and update status/scopes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (1)
packages/tempo/bin/update-version.mjs (1)
11-11: Node JSON import attributes work with repo CI (Node 22/24); addengines.nodeto prevent older local runtimes.
packages/tempo/bin/update-version.mjs:11usesimport pkg from '../package.json' with { type: 'json' };. Repo CI runs on Node 22 (.github/workflows/ci.yml) and Node 24 (.github/workflows/deploy-docs.yml), and Node’swith { type: 'json' }syntax is supported/stable in these versions—so build pipelines in CI shouldn’t break. Since noengines.nodeis declared, consider adding it to align local/off-CI usage.🤖 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/bin/update-version.mjs` at line 11, The import line "import pkg from '../package.json' with { type: 'json' }" relies on Node >=22/24 JSON import support; add an "engines" field to the package.json that encloses this script (set "engines": { "node": ">=22.0.0" } or similar) so local runtimes that are older will be prevented from running the script; update the package.json adjacent to the script (the one being imported) to include this engines.node constraint and commit the change.
🤖 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/vitest.config.ts`:
- Around line 85-86: The alias entry in vitest.config.ts currently maps the .ts
support file but not the .js path the test mocks (licenseModulePath); update the
aliasing so that when not running in dist mode you add a second resolve alias
for './src/support/support.license.js' that uses the same replacement value
(isPremiumAvailable ? licensePremium : licenseDefault) as the existing
'./src/support/support.license.ts' mapping so imports from tempo.class.ts
(import('./support/support.license.js')) and the test mock path both resolve to
the same replacement module.
---
Nitpick comments:
In `@packages/tempo/bin/update-version.mjs`:
- Line 11: The import line "import pkg from '../package.json' with { type:
'json' }" relies on Node >=22/24 JSON import support; add an "engines" field to
the package.json that encloses this script (set "engines": { "node": ">=22.0.0"
} or similar) so local runtimes that are older will be prevented from running
the script; update the package.json adjacent to the script (the one being
imported) to include this engines.node constraint and commit the change.
🪄 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: d536602d-c8cc-48a9-a0e5-f44a1d1421d6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
package.jsonpackages/library/package.jsonpackages/tempo/CHANGELOG.mdpackages/tempo/bin/update-version.mjspackages/tempo/doc/installation.mdpackages/tempo/importmap.jsonpackages/tempo/package.jsonpackages/tempo/src/module/module.parse.tspackages/tempo/src/plugin/plugin.util.tspackages/tempo/src/plugin/term/term.util.tspackages/tempo/src/support/support.init.tspackages/tempo/src/support/support.register.tspackages/tempo/src/support/support.runtime.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tempo.version.tspackages/tempo/test/plugins/licensing.full.test.tspackages/tempo/vitest.config.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/tempo/test/plugins/licensing.full.test.ts (1)
7-38: 💤 Low value
getVerifyFnandsetVerifyFnappear unused.
setVerifyFnis defined on line 29 andgetVerifyFnon line 28, but the tests override behavior viavi.mocked(Validator).mockImplementation()instead. These exports are returned fromvi.hoisted()but never invoked in this file.If these are intended for future use or debugging, consider adding a brief comment. Otherwise, they can be removed to reduce confusion.
🤖 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/plugins/licensing.full.test.ts` around lines 7 - 38, The hoisted block defines getVerifyFn and setVerifyFn (and setVerifyFn assigns currentVerify) but those functions are never used in this test file; either remove their definitions and their inclusion in the vi.hoisted() return value to avoid dead/confusing code, or keep them but add a concise comment above getVerifyFn/setVerifyFn explaining they exist for external test overrides/debugging; locate them inside the vi.hoisted() closure (referencing currentVerify, mockFactory and the vi.hoisted return) and update the return object accordingly.packages/tempo/src/tempo.class.ts (1)
394-396: 💤 Low valueRemove commented-out code.
This commented block is dead code that was intentionally removed. Consider deleting it to avoid confusion.
🤖 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/tempo.class.ts` around lines 394 - 396, Delete the dead commented-out block that references passing a license key to extendState (the commented lines checking discovery.license and merging into opts) to avoid confusion; remove the two commented lines containing "// // 1f. Pass license key through to extendState..." and "// if (discovery.license) opts = { ...opts, license: discovery.license }" in tempo.class.ts so only active code remains.packages/tempo/vitest.config.ts (1)
64-65: Potentially redundant dist self-alias in Vitest config (lines 64–65)In
packages/tempo/vitest.config.ts(whenTEST_DIST=true), the#tempo/licensealias already maps todist/plugin/license/license.validator.js(line 64). The additional entry at line 65 self-aliases that same resolved dist path to itself, but the current tests mock onlyvi.mock('#tempo/license', ...)and an absolute src path (.../src/plugin/license/license.validator.ts), not the dist absolute path (nodist/plugin/license/license.validator.jsin tests). This makes line 65 look unnecessary for the present mocking strategy; remove it unless there’s an external/other-test need tovi.mock(<absolute dist path>). If it must stay, add a comment stating that specific use case.🤖 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 64 - 65, The duplicate self-alias entry that maps the resolved dist path to itself in vitest.config.ts is redundant given the existing { find: /^`#tempo`\/license$/, replacement: resolve(... './dist/plugin/license/license.validator.js') } mapping and current tests only mock '`#tempo/license`' and the src path; remove the second alias ({ find: resolve(... 'dist/plugin/license/license.validator.js'), replacement: resolve(... 'dist/plugin/license/license.validator.js') }) to avoid unnecessary entries, or if you intentionally need tests to vi.mock the absolute dist path keep the entry and add a clarifying comment above it stating that it exists to allow vi.mock() calls using the resolved dist absolute path.
🤖 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 `@package.json`:
- Around line 5-7: The package.json engines.node constraint (engines.node:
">=22.0.0") must be clarified or justified: either relax it to ">=20.0.0" and
require loading `@js-temporal/polyfill` when globalThis.Temporal is missing, or
keep ">=22.0.0" and explicitly document why (native Temporal + Node 22 caveats).
Update package.json accordingly and add/adjust documentation in
packages/library/src/common/temporal.polyfill.ts to clearly state the runtime
requirement and fallback instructions, and update packages/tempo/CONTRIBUTING.md
and packages/tempo/CHANGELOG.md to reflect the supported Node versions and note
the engines bump and the Node 22.0.x instability warning so the runtime
constraint and migration steps are unambiguous.
In `@packages/tempo/bin/resolve-types.ts`:
- Line 68: The licReplacement value is missing the "plugin/" segment which
causes incorrect paths; update the construction of licReplacement (the variable
named licReplacement that uses prefix) to include "plugin/" before
"license/license.validator.js" so it resolves to e.g. `${prefix ||
'./'}plugin/license/license.validator.js`, ensuring the path matches the built
module location used by the vitest configuration.
In `@packages/tempo/test/discrete/standalone.test.ts`:
- Around line 7-20: Save and restore the entire runtime state (not just
state.config) and call init with the saved state as the baseState so init
doesn't early-return; e.g., replace prevState = { ...getRuntime().state?.config
} with prevState = getRuntime().state (or fullState), and in afterAll call
init(prevState?.config ?? {}, true, prevState) (or init(undefined, true,
prevState) if you prefer) so the original runtime.state is applied as baseState
and the previous configuration is actually restored (references: init(),
getRuntime(), runtime.state, prevState, afterAll).
---
Nitpick comments:
In `@packages/tempo/src/tempo.class.ts`:
- Around line 394-396: Delete the dead commented-out block that references
passing a license key to extendState (the commented lines checking
discovery.license and merging into opts) to avoid confusion; remove the two
commented lines containing "// // 1f. Pass license key through to
extendState..." and "// if (discovery.license) opts = { ...opts, license:
discovery.license }" in tempo.class.ts so only active code remains.
In `@packages/tempo/test/plugins/licensing.full.test.ts`:
- Around line 7-38: The hoisted block defines getVerifyFn and setVerifyFn (and
setVerifyFn assigns currentVerify) but those functions are never used in this
test file; either remove their definitions and their inclusion in the
vi.hoisted() return value to avoid dead/confusing code, or keep them but add a
concise comment above getVerifyFn/setVerifyFn explaining they exist for external
test overrides/debugging; locate them inside the vi.hoisted() closure
(referencing currentVerify, mockFactory and the vi.hoisted return) and update
the return object accordingly.
In `@packages/tempo/vitest.config.ts`:
- Around line 64-65: The duplicate self-alias entry that maps the resolved dist
path to itself in vitest.config.ts is redundant given the existing { find:
/^`#tempo`\/license$/, replacement: resolve(...
'./dist/plugin/license/license.validator.js') } mapping and current tests only
mock '`#tempo/license`' and the src path; remove the second alias ({ find:
resolve(... 'dist/plugin/license/license.validator.js'), replacement:
resolve(... 'dist/plugin/license/license.validator.js') }) to avoid unnecessary
entries, or if you intentionally need tests to vi.mock the absolute dist path
keep the entry and add a clarifying comment above it stating that it exists to
allow vi.mock() calls using the resolved dist absolute path.
🪄 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: 6f7bfa5f-7f64-4c30-9e24-13bef99a3220
📒 Files selected for processing (29)
package.jsonpackages/library/src/common/pledge.class.tspackages/library/src/common/scopedset.class.tspackages/library/test/common/pledge.class.test.tspackages/tempo/bench/tsconfig.jsonpackages/tempo/bin/resolve-types.tspackages/tempo/doc/installation.mdpackages/tempo/package.jsonpackages/tempo/rollup.config.jspackages/tempo/src/engine/engine.term.tspackages/tempo/src/module/module.format.tspackages/tempo/src/module/module.mutate.tspackages/tempo/src/module/module.parse.tspackages/tempo/src/plugin/license/license.manager.tspackages/tempo/src/plugin/license/license.validator.tspackages/tempo/src/plugin/plugin.util.tspackages/tempo/src/support/support.init.tspackages/tempo/src/support/support.runtime.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tsconfig.jsonpackages/tempo/src/tsconfig.repl.jsonpackages/tempo/test/core/sandbox_isolation.test.tspackages/tempo/test/discrete/standalone.test.tspackages/tempo/test/plugins/licensing.full.test.tspackages/tempo/test/plugins/licensing.sandbox.test.tspackages/tempo/test/support/setup.console-spy.tspackages/tempo/test/tsconfig.jsonpackages/tempo/vitest.config.ts
✅ Files skipped from review due to trivial changes (1)
- packages/library/src/common/scopedset.class.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/tempo/src/tempo.type.ts
- packages/tempo/src/plugin/plugin.util.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/library/src/common/temporal.polyfill.ts`:
- Around line 14-16: The error string in temporal.polyfill.ts currently
recommends "Node 22+, Deno, Bun" but then warns Node 22.0.x is unstable; update
that message to align with the CHANGELOG and docs by recommending "Node 20+ with
a polyfill (e.g. `@js-temporal/polyfill`) is highly recommended" and note that
native Temporal is fully reliable only on Node 26+ (while Node 22 may have
instabilities), so replace the existing message text in the file's
Temporal/polyfill error string to reflect: Node 20+ with polyfill recommended,
mention Node 22 instability, and indicate Node 26+ ships Temporal fully enabled.
Ensure you modify the literal error message constant/string in
temporal.polyfill.ts (the message used when Temporal is unavailable).
In `@packages/tempo/test/browser/licensing.browser.test.ts`:
- Around line 23-25: Replace the brittle provider-specific text check against
'verify' on the test result object `res`: instead of asserting
expect(res.error).toContain('verify'), assert stable semantics such as the
validation status and non-empty error message — e.g. verify `res.status` equals
'invalid' (expect(res.status).toBe('invalid')) and keep the existing checks that
`res.error` is defined and does not contain 'Community Build'.
🪄 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: a9f2b24f-c91d-4a77-b89b-fc524601743a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
package.jsonpackages/library/src/common/temporal.polyfill.tspackages/tempo/CHANGELOG.mdpackages/tempo/CONTRIBUTING.mdpackages/tempo/bin/resolve-types.tspackages/tempo/package.jsonpackages/tempo/src/tempo.class.tspackages/tempo/test/browser/bundle.browser.test.tspackages/tempo/test/browser/licensing.browser.test.tspackages/tempo/test/discrete/standalone.test.tspackages/tempo/test/plugins/licensing.full.test.tspackages/tempo/vitest.browser.config.tspackages/tempo/vitest.config.ts
💤 Files with no reviewable changes (1)
- packages/tempo/src/tempo.class.ts
✅ Files skipped from review due to trivial changes (3)
- packages/tempo/CONTRIBUTING.md
- packages/tempo/vitest.browser.config.ts
- packages/tempo/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/tempo/bin/resolve-types.ts
- packages/tempo/test/discrete/standalone.test.ts
- packages/tempo/package.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/tempo/src/plugin/term/term.zodiac.ts (1)
115-115: ⚡ Quick winConsider tightening the
yinYangtype to match runtime constraints.Line 85 in
getChineseZodiaccomputesyinYangasyear % 2 === 0 ? 'Yang' : 'Yin', so the value is always one of these two literals. The current typeyinYang: stringis safe but overly permissive.🔧 Suggested refinement
CN?: { animal: string; traits: string; element: string; - yinYang: string; + yinYang: 'Yang' | 'Yin'; };🤖 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/term/term.zodiac.ts` at line 115, The yinYang property is typed too broadly as string even though getChineseZodiac computes it as either 'Yang' or 'Yin'; tighten the type to the literal union 'Yin' | 'Yang' on the yinYang property and any related return/interface/type declarations (e.g., the return type of getChineseZodiac and the object shape that contains yinYang) so the type system reflects the actual runtime values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tempo/src/plugin/term/term.timeline.ts`:
- Around line 42-46: The period object returned by getTermRange for the timeline
term only ever contains hour (and minute for “afternoon”) and therefore the
TempoTermRegistry.period typing is misleading; update the
TempoTermRegistry.period type to reflect the actual shape returned by
term.timeline.ts (e.g., define a specific TimelinePeriod interface with only
hour?: number and minute?: number or a union for entries that only have hour) or
add a clear doc comment on TempoTermRegistry.period stating sub-second fields
(second, millisecond, microsecond, nanosecond) will not be present for the
timeline term; locate the timeline groups in term.timeline.ts and the
getTermRange usage to ensure the typing change aligns with the runtime shape.
---
Nitpick comments:
In `@packages/tempo/src/plugin/term/term.zodiac.ts`:
- Line 115: The yinYang property is typed too broadly as string even though
getChineseZodiac computes it as either 'Yang' or 'Yin'; tighten the type to the
literal union 'Yin' | 'Yang' on the yinYang property and any related
return/interface/type declarations (e.g., the return type of getChineseZodiac
and the object shape that contains yinYang) so the type system reflects the
actual runtime values.
🪄 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: f106f577-c01a-4752-a1f2-bab8778a07b4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.github/workflows/ci.ymlpackages/library/src/common/temporal.polyfill.tspackages/tempo/src/plugin/term/term.quarter.tspackages/tempo/src/plugin/term/term.season.tspackages/tempo/src/plugin/term/term.timeline.tspackages/tempo/src/plugin/term/term.zodiac.tspackages/tempo/src/tempo.class.tspackages/tempo/test/browser/bundle.browser.test.tspackages/tempo/test/browser/licensing.browser.test.ts
💤 Files with no reviewable changes (2)
- packages/tempo/test/browser/licensing.browser.test.ts
- packages/tempo/test/browser/bundle.browser.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/library/src/common/temporal.polyfill.ts
- packages/tempo/src/tempo.class.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation