Release/v2.2.2#14
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughRelease v2.2.2 introducing dist-based testing infrastructure, refactoring term registries to persist globally across module reloads, enhancing prototype chain traversal in reflection utilities, and redesigning documentation with updated Vue markup and hero sections. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
packages/tempo/.npmignore (1)
1-11: Redundant withfileswhitelist inpackage.json.
packages/tempo/package.jsonalready sets"files": ["dist/"], which is a whitelist and takes precedence — npm will only publishdist/(plus always-included files likepackage.json,README.md,LICENSE) regardless of.npmignore. The.npmignorehere is effectively dead configuration. Consider removing it to avoid drift (e.g., someone later adds an entry tofilesand forgets to update.npmignore, or vice versa).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/.npmignore` around lines 1 - 11, The .npmignore file is redundant because package.json already uses the "files": ["dist/"] whitelist which controls published contents; remove packages/tempo/.npmignore to avoid configuration drift and rely on the "files" field in package.json (keep README.md, LICENSE, package.json as they are always included).packages/tempo/src/tempo.register.ts (1)
15-23: Minor inconsistency in how registry slots are shared across reloads.
_terms/_extends(Lines 15-16): store the raw array onglobalThis; each module load wraps it in a freshsecureRef. ✅ Shared data, local proxy.modules(Line 21): stores thesecureRefproxy itself onglobalThis. On a subsequent module load, the first load's proxy (with its captured closures) is reused, not a fresh wrapper.installed(Line 22): stores the rawSet. ✅ Shared data.The
modulesslot is the odd one out — it retains the first-ever module's proxy state across reloads, which can keep the first module alive via closure references and diverges from the pattern used byterms/extends. Consider storing the raw record onglobalThisand wrapping locally, consistent with_terms/_extends:♻️ Suggested alignment
-const _terms = (globalThis as any)[sym.$terms] ??= [] as TermPlugin[]; -const _extends = (globalThis as any)[sym.$extends] ??= [] as Extension[]; +const _terms = ((globalThis as any)[sym.$terms] ??= [] as TermPlugin[]); +const _extends = ((globalThis as any)[sym.$extends] ??= [] as Extension[]); +const _modules = ((globalThis as any)[sym.$modules] ??= {} as Record<string, any>); const _REGISTRY = { terms: secureRef(_terms), extends: secureRef(_extends), - modules: (globalThis as any)[sym.$modules] ??= secureRef({} as Record<string, any>), + modules: secureRef(_modules), installed: (globalThis as any)[sym.$installed] ??= new Set() }Side benefit:
registryResetcan drop theinternal.modules[lib.$Target] ?? internal.modulesfallback on Line 76 since the unwrap becomes uniform.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/tempo.register.ts` around lines 15 - 23, The registry stores a secureRef for modules on globalThis while storing raw arrays for _terms and _extends, causing the first module load's proxy closures to persist; change the modules slot to store the raw record on globalThis (like _terms/_extends) and then wrap it locally with secureRef when building _REGISTRY so that secureRef({}) is not reused across reloads; update references to symbols _terms, _extends, modules, secureRef and _REGISTRY and adjust registryReset (and the internal.modules lookup / fallback such as the lib.$Target usage) to assume the unwrap is uniform after this change.packages/tempo/rollup.config.js (1)
13-36: Robustness of dynamic entry discovery.Two minor concerns with the new dist-driven
inputgeneration:
Hard failure when
dist/is absent.fs.readdirSync('dist', ...)throws synchronously at config load time. Thebuildscript runstsc -bfirst so this is fine in the happy path, butrollup -cin isolation (e.g. debuggingbuild:bundleafterclean) now produces an opaqueENOENT. A guarded early return with a clear message would help:💡 Guard example
-// Generate a map of entry points from all files in dist (after tsc has run) -const entryPoints = Object.fromEntries( - getFiles('dist').map(file => [ - path.relative('dist', file).replace(/\.js$/, ''), - file - ]) -); +// Generate a map of entry points from all files in dist (after tsc has run) +if (!fs.existsSync('dist')) { + throw new Error('rollup: dist/ not found — run `tsc -b` (or `npm run build`) first.'); +} +const entryPoints = Object.fromEntries( + getFiles('dist').map(file => [ + path.relative('dist', file).replace(/\.js$/, ''), + file + ]) +);CWD-relative paths. Both
getFiles('dist')andpath.resolve('../library/dist/common.index.js')depend onprocess.cwd()beingpackages/tempo. That holds when invoked through workspace scripts but silently breaks when invoked from the monorepo root directly. Anchoring againstimport.meta.url(this is an ESM config —import path from 'node:path') would make it cwd-independent:import { fileURLToPath } from 'node:url'; const here = path.dirname(fileURLToPath(import.meta.url)); // ...getFiles(path.join(here, 'dist')) and path.resolve(here, '../library/dist/common.index.js')Neither blocks the release; both reduce future friction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/rollup.config.js` around lines 13 - 36, The dynamic entry discovery in getFiles and entryPoints can fail when the dist directory is missing and is cwd-dependent; update getFiles (and its call site that builds entryPoints) to first compute a repo-local base directory using import.meta.url (via fileURLToPath and path.dirname) and use path.join(here, 'dist') instead of a raw 'dist', and add a guarded check that the dist directory exists (fs.existsSync or try/catch around fs.readdirSync) that returns an empty map or throws a clear, descriptive error instead of letting ENOENT bubble; also resolve the forced inclusion path (entryPoints['lib/common.index']) against the same here (e.g., path.resolve(here, '../library/dist/common.index.js')) so all paths are independent of process.cwd().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/library/src/common/reflection.library.ts`:
- Around line 102-112: The constructor-chain loop in reflection.library.ts is
terminating against Function and Object constructors, which lets
Function.prototype and Object.prototype be iterated and leak keys like
'__proto__' and 'arguments'; update the loop termination in the code that walks
the constructor chain (the variable named "constructor" and the while condition
that currently checks "constructor !== Object && constructor !== Function &&
++depth < limit") to mirror the instance-chain logic by stopping at the
prototype objects themselves (e.g., check "constructor !== Function.prototype &&
constructor !== Object.prototype") and ensure the loop also respects null, so
that Object.getPrototypeOf(constructor) will terminate without collecting
descriptors from Function.prototype/Object.prototype. This change prevents
collection of prototype-only accessors such as '__proto__' and 'arguments'.
In `@packages/tempo/.vitepress/config.ts`:
- Around line 77-91: The VitePress alias configuration (the alias entries
referencing ../dist/... and the `@magmacomputing/tempo` replacements) creates an
implicit dependency on compiled artifacts, so update the package scripts that
run VitePress (the docs:dev and docs:build scripts in packages/tempo
package.json) to run build:library and build:tempo (or a new predocs script)
before docs:api and the VitePress step, or alternatively add clear instructions
to CONTRIBUTING/README about requiring build:library and build:tempo prior to
running docs:dev/docs:build; adjust the scripts so the dist artifacts exist
before vitepress starts to avoid unresolved import errors.
In `@packages/tempo/index.md`:
- Around line 23-49: The onMounted handler awaits dynamic imports then creates a
recurring Tempo.ticker but onUnmounted may have already fired, causing a leaked
timer; fix by adding a local cancellation flag (e.g., let cancelled = false) or
an isMounted boolean: set it true at the start of onMounted and false in
onUnmounted, then after the Promise.all and before calling Tempo.ticker(...)
check the flag and skip creating the ticker if cancelled; additionally, if a
ticker is created ensure onUnmounted always stops it (ticker?.stop()) and nulled
to avoid any dangling references.
In `@packages/tempo/package.json`:
- Around line 150-151: The change removed the workspace-based Vitest invocation
(vitest.workspace.ts) and consolidated tests into the local vitest.config.ts,
altering test suites from two isolated runs ("Tempo: Full" vs "Tempo: Core") to
a single pooled run; confirm this behavioral change is intentional and, if you
want to preserve previous suite separation, revert to invoking the workspace
config or recreate equivalent exclusion/inclusion patterns in vitest.config.ts
(look for the "test" script and vitest.workspace.ts / vitest.config.ts
references), and also add cross-env to this package's devDependencies so the
"test:dist" script (which uses cross-env) works when the package is installed
standalone (update package.json devDependencies to include "cross-env").
In `@packages/tempo/README.md`:
- Around line 4-10: Inline style attributes on the HTML table/header (elements:
<td>, <img src="./img/logo.svg">, <h1>, <p>) will be stripped by GitHub/npm
renderers; remove reliance on inline styles in the README and instead use
GitHub-supported markup or assets: replace the styled <h1 style=...> with a
plain Markdown heading (e.g. "# Tempo") or use <h1 align="left">, convert the
styled <p> to a normal paragraph without inline CSS, keep the image but if you
need the exact visual banner create a pre-rendered banner image and reference it
via the existing <img src="./img/logo.svg">, or move style rules into the
VitePress site stylesheet for docs-only styling.
In `@packages/tempo/src/plugin/term/standard.index.ts`:
- Around line 7-11: The resetHooks Set accumulates callbacks because each import
registers a new function reference; modify the registry reset handling so after
invoking each hook from resetHooks() you clear the Set to prevent accumulation —
i.e., in the reset routine that currently calls resetHooks().forEach(hook =>
hook()), add a call to resetHooks().clear() immediately after; this touches the
reset logic that invokes resetHooks and ensures repeated imports/HMR do not grow
the hook Set while preserving existing behavior of onRegistryReset and
Tempo.extend re-extension.
---
Nitpick comments:
In `@packages/tempo/.npmignore`:
- Around line 1-11: The .npmignore file is redundant because package.json
already uses the "files": ["dist/"] whitelist which controls published contents;
remove packages/tempo/.npmignore to avoid configuration drift and rely on the
"files" field in package.json (keep README.md, LICENSE, package.json as they are
always included).
In `@packages/tempo/rollup.config.js`:
- Around line 13-36: The dynamic entry discovery in getFiles and entryPoints can
fail when the dist directory is missing and is cwd-dependent; update getFiles
(and its call site that builds entryPoints) to first compute a repo-local base
directory using import.meta.url (via fileURLToPath and path.dirname) and use
path.join(here, 'dist') instead of a raw 'dist', and add a guarded check that
the dist directory exists (fs.existsSync or try/catch around fs.readdirSync)
that returns an empty map or throws a clear, descriptive error instead of
letting ENOENT bubble; also resolve the forced inclusion path
(entryPoints['lib/common.index']) against the same here (e.g.,
path.resolve(here, '../library/dist/common.index.js')) so all paths are
independent of process.cwd().
In `@packages/tempo/src/tempo.register.ts`:
- Around line 15-23: The registry stores a secureRef for modules on globalThis
while storing raw arrays for _terms and _extends, causing the first module
load's proxy closures to persist; change the modules slot to store the raw
record on globalThis (like _terms/_extends) and then wrap it locally with
secureRef when building _REGISTRY so that secureRef({}) is not reused across
reloads; update references to symbols _terms, _extends, modules, secureRef and
_REGISTRY and adjust registryReset (and the internal.modules lookup / fallback
such as the lib.$Target usage) to assume the unwrap is uniform after this
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: 0f014d5e-5020-42b0-8d84-556d2ffec95a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
package.jsonpackages/library/package.jsonpackages/library/src/common/pledge.class.tspackages/library/src/common/reflection.library.tspackages/library/vitest.config.tspackages/tempo/.npmignorepackages/tempo/.vitepress/config.tspackages/tempo/CHANGELOG.mdpackages/tempo/README.mdpackages/tempo/doc/tempo.ticker.mdpackages/tempo/index.mdpackages/tempo/package.jsonpackages/tempo/rollup.config.jspackages/tempo/src/plugin/plugin.util.tspackages/tempo/src/plugin/term/standard.index.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.register.tspackages/tempo/src/tempo.symbol.tspackages/tempo/vitest.config.tsvitest.workspace.ts
💤 Files with no reviewable changes (1)
- packages/tempo/src/tempo.class.ts
| "test": "vitest run", | ||
| "test:dist": "cross-env TEST_DIST=true vitest run", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the split config previously excluded these tests and find any that rely on isolation
rg -nP --type=ts -C2 "core\.test|lazy\.test" packages/tempo/test 2>/dev/null | head -60
# Check whether cross-env is declared in tempo package devDependencies
cat packages/tempo/package.json | jq '.devDependencies."cross-env" // "NOT DECLARED"'Repository: magmacomputing/magma
Length of output: 78
🏁 Script executed:
#!/bin/bash
# Check if vitest.workspace.ts exists
find . -name "vitest.workspace.ts" -type f
# Look for any test files with .core.test or .lazy.test patterns anywhere in repo
find . -name "*.core.test.ts" -o -name "*.lazy.test.ts" 2>/dev/null
# Check the current vitest.config.ts in packages/tempo
cat packages/tempo/vitest.config.tsRepository: magmacomputing/magma
Length of output: 4682
🏁 Script executed:
#!/bin/bash
# Read the workspace config to see what it was doing
cat vitest.workspace.tsRepository: magmacomputing/magma
Length of output: 1198
Confirm intentional test execution changes and add missing dependency.
The removal of --workspace ../../vitest.workspace.ts changes test execution behavior significantly. Previously, the workspace config ran tests in two separate suites: "Tempo: Full" (excluding *.core.test.ts and *.lazy.test.ts) and "Tempo: Core" (including only those). The new local vitest.config.ts runs all tests together via a single pool with pool: 'forks' for process isolation. While fork isolation mitigates shared-state risks, this differs from the intentional suite separation. Confirm this behavioral change is intentional and that no unexpected test interactions occur.
Additionally, test:dist uses cross-env but it is not declared in packages/tempo/devDependencies. This dependency exists only at the root, so the script will fail if the package is installed or tested standalone. Add cross-env to this package's devDependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/tempo/package.json` around lines 150 - 151, The change removed the
workspace-based Vitest invocation (vitest.workspace.ts) and consolidated tests
into the local vitest.config.ts, altering test suites from two isolated runs
("Tempo: Full" vs "Tempo: Core") to a single pooled run; confirm this behavioral
change is intentional and, if you want to preserve previous suite separation,
revert to invoking the workspace config or recreate equivalent
exclusion/inclusion patterns in vitest.config.ts (look for the "test" script and
vitest.workspace.ts / vitest.config.ts references), and also add cross-env to
this package's devDependencies so the "test:dist" script (which uses cross-env)
works when the package is installed standalone (update package.json
devDependencies to include "cross-env").
1 participant
Emergency Patch to correct some missed library functionality
Summary by CodeRabbit
Bug Fixes
Documentation
Tests