Release v0.8.0 #8
Conversation
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughThis pull request converts the Alfred project from a single package into a pnpm workspace containing two packages: Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant CommandRouter
participant ConfigRegistry
participant Adaptive
participant Codec
Note over Client,Codec: Read Config Operation
Client->>CommandRouter: execute({type: 'read_config', path: 'retry/count'})
CommandRouter->>ConfigRegistry: read('retry/count')
ConfigRegistry->>Adaptive: get()
Adaptive-->>ConfigRegistry: value
ConfigRegistry->>Codec: format(value)
Codec-->>ConfigRegistry: formatted string
ConfigRegistry-->>CommandRouter: Result{ok: true, snapshot}
CommandRouter-->>Client: Result{ok: true, snapshot}
Note over Client,Codec: Write Config Operation
Client->>CommandRouter: execute({type: 'write_config', path: 'retry/count', value: '5'})
CommandRouter->>ConfigRegistry: write('retry/count', '5')
ConfigRegistry->>Codec: parse('5')
Codec-->>ConfigRegistry: parsed value
ConfigRegistry->>Adaptive: set(parsed value)
Adaptive-->>ConfigRegistry: ✓
ConfigRegistry->>Adaptive: get()
Adaptive-->>ConfigRegistry: updated value
ConfigRegistry->>Codec: format(updated value)
Codec-->>ConfigRegistry: formatted string
ConfigRegistry-->>CommandRouter: Result{ok: true, snapshot}
CommandRouter-->>Client: Result{ok: true, snapshot}
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 4
🤖 Fix all issues with AI agents
In @.github/workflows/publish.yml:
- Around line 18-35: Generate and commit pnpm-lock.yaml to the repo before
enabling frozen lockfiles, then update the workflow steps that run pnpm install:
run pnpm install locally to create pnpm-lock.yaml and commit it, and modify the
"Install Dependencies" step (the step named "Install Dependencies" that
currently runs pnpm install) to run pnpm install --frozen-lockfile so CI uses
the committed lockfile for reproducible releases; ensure NODE_AUTH_TOKEN is
preserved for the "Publish Packages" step.
In `@alfred/package.json`:
- Around line 26-31: The scripts "test:browser", "demo:web" and
"demo:web:install" in package.json reference a non-existent examples/web
directory; either add that examples/web package (with its package.json and any
needed lockfile or adapt to monorepo tooling) so those npm --prefix commands
work, or remove/rename these scripts (or convert them to appropriate pnpm
workspace commands like using --filter) so package.json does not call a missing
path; update the entries "test:browser", "demo:web" and "demo:web:install"
accordingly.
In `@scripts/release/bump.mjs`:
- Around line 66-72: The current catch around reading/writing jsr.json swallows
all errors; update the try/catch around jsrPath/readJson/writeJson so only a
missing-file error is ignored (check err.code === 'ENOENT' or equivalent), and
rethrow or surface any other errors (e.g., JSON parse failures or permission
errors) so jsr.json stays in sync; reference jsrPath, readJson, writeJson and
nextVersion to locate the block and implement the narrowed catch.
In `@scripts/release/preflight.mjs`:
- Around line 51-62: The exports-array branch in getExportsMap incorrectly
stores the raw array as allTargets so string targets aren't validated; modify
getExportsMap so when exportsField is an array you call
collectStringTargets(exportsField) and use the resulting set for allTargets (and
still derive the default via pickDefaultTarget), and for the string case keep a
Set([exportsField]) — update the block that creates the map entry for '.' to use
collectStringTargets for arrays and ensure allTargets is a Set of strings.
🧹 Nitpick comments (7)
.gitignore (1)
6-6: Redundant ignore pattern.The pattern
test-results/on line 4 already matchesalfred/examples/web/test-results/at any directory level. Line 6 can be removed.🧹 Simplify by removing the redundant pattern
-alfred/examples/web/test-results/alfred/eslint.config.js (1)
62-62: Minor: Redundant file pattern.
test/**/*.test.jsis already matched bytest/**/*.js, making it redundant.🔧 Suggested simplification
- files: ['test/**/*.js', 'test/**/*.test.js'], + files: ['test/**/*.js'],scripts/release/workspace.mjs (1)
33-55: Current patterns don't use nested paths, but the code cannot handle them.The patterns in
pnpm-workspace.yamlare"alfred"and"alfred-*", both top-level, so they work correctly. However, if someone adds a nested pattern likepackages/*, the regex test would fail silently since top-level directory names cannot contain/. This would result in the error "No workspace packages found" without clarifying the root cause.Consider adding a guard to detect and reject nested patterns explicitly, or document this limitation.
Guard example
for (const pattern of patterns) { + if (pattern.includes('/')) { + throw new Error(`Nested patterns not supported; use top-level patterns only (found "${pattern}").`); + } if (!pattern.includes('*')) { matched.add(pattern); continue; }alfred-live/README.md (1)
47-52: Consider varying the bullet sentence starts for readability.✍️ Optional wording tweak
-- `bulkhead` matches `bulkhead` and `bulkhead/api`. -- `bulkhead/*` matches children only (e.g. `bulkhead/api`), not `bulkhead` itself. -- `bulkhead*` matches `bulkhead` and `bulkhead2`. +- Prefix `bulkhead` matches `bulkhead` and `bulkhead/api`. +- Pattern `bulkhead/*` matches children only (e.g. `bulkhead/api`), not `bulkhead` itself. +- Wildcard `bulkhead*` matches `bulkhead` and `bulkhead2`.COOKBOOK.md (1)
82-97: Add input validation to theparsefunction.The parse function silently accepts invalid input, returning
NaNfor non-numeric strings. This inconsistency with Recipe 2 (Lines 53-56) could mislead users.📝 Suggested improvement
registry.register('bulkhead/limit', bulkheadLimit, { - parse: (value) => Number(value), + parse: (value) => { + const parsed = Number(value); + if (!Number.isFinite(parsed)) throw new Error('bulkhead/limit must be a number'); + return parsed; + }, format: (value) => value.toString(), });README.md (1)
33-33: Consider using underscores for emphasis consistency.Static analysis flagged inconsistent emphasis style. The rest of the document should be checked for consistency.
📝 Suggested fix
-- *A release bumps every package even if only one changed.* +- _A release bumps every package even if only one changed._alfred-live/src/registry.js (1)
95-115: Consider using a distinct error type for Adaptive validation.
validateAdaptiveusesInvalidCodecErrorfor Adaptive validation failures. This is semantically misleading since Adaptive and Codec are separate concepts. Consider introducing anInvalidAdaptiveErroror using a more generic validation error.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@alfred-live/src/registry.js`:
- Around line 117-133: createSnapshot currently calls entry.adaptive.get()
directly so any exception bubbles up; wrap the call to entry.adaptive.get() in a
try/catch and return errorResult(new ValidationError('Failed to format value.',
{ path, error: String(error) })) on failure (instead of throwing), then proceed
to call entry.format within the same try so formatting errors are also captured;
likewise locate the code handling entry.adaptive.set (the related snapshot-apply
function around the other diff region) and wrap entry.adaptive.set in a
try/catch that returns an appropriate errorResult with a ValidationError
containing the path and String(error) so all adaptive get/set failures preserve
the Result API.
In `@scripts/release/preflight.mjs`:
- Around line 64-71: Detect when exportsField is a conditional-root export
(i.e., it's an object whose keys are only condition names and none start with
'.'), and normalize it to a single '.' entry before the loop so the rest of the
logic treats it as a subpath. Concretely: if exportsField is an object and
Object.keys(exportsField).every(k => k === '.' || !k.startsWith('.')) and there
is no literal subpath key (no key that starts with '.' except '.' itself),
replace or wrap exportsField with an object whose '.' property contains the
original exportsField (merging conditions into that '.' entry); then continue
using collectStringTargets(value), pickDefaultTarget(value) and setting
map.set(key, ...) as before so conditional-root exports are handled like a '.'
subpath.
🧹 Nitpick comments (6)
.github/workflows/ci.yml (1)
19-30: Add--frozen-lockfiletopnpm installfor deterministic CI builds.The
pnpm-lock.yamlis committed to the repository. Usingpnpm install --frozen-lockfilein CI will fail immediately if the lockfile is out of sync withpackage.json, catching drift early rather than masking it with silent updates. This is especially important in this monorepo given the explicit shared versioning policy.♻️ Proposed change
- - run: pnpm install + - run: pnpm install --frozen-lockfileApply the same change to
.github/workflows/publish.ymlas well.alfred-live/README.md (1)
47-51: Reduce repeated sentence starts in Path Semantics bullets.Optional doc polish to avoid repetitive phrasing.
💡 Suggested rewording
-- `bulkhead` matches `bulkhead` and `bulkhead/api`. -- `bulkhead/*` matches children only (e.g. `bulkhead/api`), not `bulkhead` itself. -- `bulkhead*` matches `bulkhead` and `bulkhead2`. +- Exact prefix: `bulkhead` matches `bulkhead` and `bulkhead/api`. +- Child-only wildcard: `bulkhead/*` matches children only (e.g. `bulkhead/api`), not `bulkhead` itself. +- Glob-style prefix: `bulkhead*` matches `bulkhead` and `bulkhead2`.alfred-live/test/unit/adaptive.test.js (1)
5-15: Consider strengthening the timestamp assertion.The assertion
toBeGreaterThanOrEqual(initialUpdatedAt)at line 14 is technically correct but weak—ifset()executes within the same millisecond,updatedAtcould equalinitialUpdatedAt, making the test pass without proving the timestamp was refreshed.A more robust approach would introduce a small delay or mock
Date.now()to ensure the timestamp changes:💡 Optional: Use fake timers for deterministic timestamp testing
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { describe, it, expect } from 'vitest'; import { Adaptive } from '../../src/index.js'; describe('Adaptive', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + it('tracks version and updatedAt', () => { const adaptive = new Adaptive('alpha'); const initialVersion = adaptive.version; const initialUpdatedAt = adaptive.updatedAt; + vi.advanceTimersByTime(100); adaptive.set('beta'); expect(adaptive.get()).toBe('beta'); expect(adaptive.version).toBe(initialVersion + 1); - expect(adaptive.updatedAt).toBeGreaterThanOrEqual(initialUpdatedAt); + expect(adaptive.updatedAt).toBeGreaterThan(initialUpdatedAt); });COOKBOOK.md (1)
82-97: Inconsistent validation in parse function compared to other recipes.Recipe 3's parse function at line 88 uses
Number(value)without validation, which would silently passNaNfor invalid input like'abc'. This is inconsistent with Recipe 2 (line 55) and the example inalfred-live/examples/control-plane/basic.js, which both validate usingNumber.isFinite.For consistency and to demonstrate best practices:
📝 Suggested fix for consistent validation
const router = new CommandRouter(registry); registry.register('bulkhead/limit', bulkheadLimit, { - parse: (value) => Number(value), + parse: (value) => { + const parsed = Number(value); + if (!Number.isFinite(parsed)) throw new Error('bulkhead/limit must be a number'); + return parsed; + }, format: (value) => value.toString(), });scripts/release/publish.mjs (1)
6-11: Consider logging the failed command for easier debugging.When
run()exits on failure, there's no indication of which command failed. Adding a brief log message before exiting would help diagnose CI failures.💡 Optional: Add failure logging
function run(cmd, args, options = {}) { const result = spawnSync(cmd, args, { stdio: 'inherit', ...options }); if (result.status !== 0) { + console.error(`Command failed: ${cmd} ${args.join(' ')}`); process.exit(result.status ?? 1); } }scripts/release/preflight.mjs (1)
30-49:files[]matching does not support glob patterns.The code treats entries like
dist/**as literal paths. While no glob patterns are currently used in the repository's package.json files, this is a real limitation that could cause false negatives if glob patterns are added in the future. Consider using minimatch or npm-packlist to properly expand glob patterns, or at minimum detect glob characters and skip the strict prefix-match check for entries that contain them.
1 participant
Highlights:
Tests:
Summary by CodeRabbit
Release Notes
New Features
alfred-livepackage with in-process configuration management capabilitiesAdaptivefor reactive value holders with versioning and timestampsConfigRegistryfor managing path-based configuration entries with parse/format supportCommandRouterfor executing configuration read, write, and list operationsDocumentation
Chores