Add API diff tooling for v3-to-v4 migration#6547
Conversation
- Detect and report module and export changes across worktrees - Generate machine-readable and Markdown v3-to-v4 migration maps
🦋 Changeset detectedLatest commit: 8ed44c3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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:
📝 WalkthroughWalkthroughThis PR adds a new internal ChangesAPI diff tool and migration mappings
API diff tool design specification
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Cli
participant ApiDiff
participant Worktrees
participant Snapshotter
participant DiffModule as Diff
participant Report
Cli->>ApiDiff: run({ baseRef, headRef, output })
ApiDiff->>Worktrees: resolveRef(baseRef)
ApiDiff->>Worktrees: resolveRef(headRef)
ApiDiff->>Worktrees: prepareSnapshot(base options)
Worktrees->>Snapshotter: extract(entrypoints)
Snapshotter-->>Worktrees: ApiSnapshot (base)
ApiDiff->>Worktrees: prepareSnapshot(head options)
Worktrees->>Snapshotter: extract(entrypoints)
Snapshotter-->>Worktrees: ApiSnapshot (head)
ApiDiff->>DiffModule: diffSnapshots(base, head)
DiffModule-->>ApiDiff: ApiDiff changes
ApiDiff->>Report: renderMarkdownReport(diff)
Report-->>ApiDiff: markdown report
ApiDiff->>ApiDiff: write base/head/diff/report files
Suggested labels: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
- Deduplicate rendered API rename entries - Add coverage for grep-friendly migration mappings
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/tools/api-diff/src/Discovery.ts (1)
55-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant ternary — both branches are identical.
declarationRootevaluates tojoin(sourceRoot, "dist")regardless ofpacked, so the conditional serves no purpose. If a difference was intended (e.g., non-distdeclaration root for non-packed packages), this looks unfinished; otherwise simplify.♻️ Simplify if no per-
packeddifference is intended- declarationRoot: packed ? join(sourceRoot, "dist") : join(sourceRoot, "dist"), + declarationRoot: join(sourceRoot, "dist"),🤖 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/tools/api-diff/src/Discovery.ts` around lines 55 - 64, In the package metadata mapping that constructs PackageInfo, simplify declarationRoot to a single join(sourceRoot, "dist") expression because both packed branches are identical; leave root and exports conditional logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tools/api-diff/tsconfig.json`:
- Around line 1-8: Update the api-diff TypeScript configuration to include the
test files under test/*.test.ts in type-checking, using the repository’s
established test tsconfig or project-reference pattern if available. Preserve
the existing src inclusion and Node types configuration while ensuring pnpm
check covers the tests.
---
Nitpick comments:
In `@packages/tools/api-diff/src/Discovery.ts`:
- Around line 55-64: In the package metadata mapping that constructs
PackageInfo, simplify declarationRoot to a single join(sourceRoot, "dist")
expression because both packed branches are identical; leave root and exports
conditional logic unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c6e0eb7-d8e3-4f84-b4d5-d3415a01649d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
migration/v3-to-v4.jsonmigration/v3-to-v4.mdpackage.jsonpackages/tools/api-diff/README.mdpackages/tools/api-diff/package.jsonpackages/tools/api-diff/src/Cli.tspackages/tools/api-diff/src/Diff.tspackages/tools/api-diff/src/Discovery.tspackages/tools/api-diff/src/Json.tspackages/tools/api-diff/src/Mapping.tspackages/tools/api-diff/src/Model.tspackages/tools/api-diff/src/Report.tspackages/tools/api-diff/src/Snapshot.tspackages/tools/api-diff/src/Worktrees.tspackages/tools/api-diff/src/bin.tspackages/tools/api-diff/test/Diff.test.tspackages/tools/api-diff/test/Discovery.test.tspackages/tools/api-diff/test/Mapping.test.tspackages/tools/api-diff/test/Snapshot.test.tspackages/tools/api-diff/test/utils.tspackages/tools/api-diff/tsconfig.jsonpackages/tools/api-diff/vitest.config.tsplan.mdtsconfig.jsontsconfig.packages.json
| { | ||
| "$schema": "http://json.schemastore.org/tsconfig", | ||
| "extends": "../../../tsconfig.base.json", | ||
| "include": ["src"], | ||
| "compilerOptions": { | ||
| "types": ["node"] | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare tsconfig "include" patterns across sibling tool/package configs
fd tsconfig.json packages -x cat -n {}Repository: Effect-TS/effect
Length of output: 9568
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== api-diff package files =="
fd -a . packages/tools/api-diff -t f | sort
echo
echo "== package.json =="
cat -n packages/tools/api-diff/package.json
echo
echo "== tsconfig files =="
fd -a 'tsconfig*.json' packages/tools/api-diff -t f | sort | while read -r f; do
echo "--- $f ---"
cat -n "$f"
doneRepository: Effect-TS/effect
Length of output: 2311
Type-check the test files too. packages/tools/api-diff/tsconfig.json only includes src, so pnpm check skips test/*.test.ts; add a test tsconfig/reference if these should stay type-safe.
🤖 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/tools/api-diff/tsconfig.json` around lines 1 - 8, Update the
api-diff TypeScript configuration to include the test files under test/*.test.ts
in type-checking, using the repository’s established test tsconfig or
project-reference pattern if available. Preserve the existing src inclusion and
Node types configuration while ensuring pnpm check covers the tests.
github-project-automation
Bot
moved this from Discussion Ongoing
to Waiting on Author
in PR Backlog
- Run the command with Effect platform services - Add coverage for mapping documentation generation
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tools/api-diff/test/Cli.test.ts`:
- Around line 13-27: Update the test containing the temporary directory setup
and Command.runWith invocation to wrap the test body in try/finally, and call
rmSync(root, { recursive: true, force: true }) in the finally block so cleanup
runs on both success and failure.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a4fc3742-ad5a-496a-9f2e-75f143de512a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.changeset/migrate-api-diff-cli.mdpackages/tools/api-diff/package.jsonpackages/tools/api-diff/src/Cli.tspackages/tools/api-diff/src/bin.tspackages/tools/api-diff/test/Cli.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tools/api-diff/package.json
- Use Effect platform services for discovery, snapshots, worktrees, and CLI execution - Validate manifests, migration maps, and snapshots with Schema
- Report unmatched base and head APIs as removals and additions - Suggest cross-module replacements while preserving class facets
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tools/api-diff/src/Discovery.ts (1)
140-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant conditional on
declarationRoot. Both branches of the ternary producepath.join(sourceRoot, "dist"), so thepackedtest has no effect here. Either collapse it, or—if the non-packed case was meant to resolve declarations somewhere other thandist—fix the intended branch.♻️ Suggested simplification (if `dist` is correct for both)
- declarationRoot: packed ? path.join(sourceRoot, "dist") : path.join(sourceRoot, "dist"), + declarationRoot: path.join(sourceRoot, "dist"),🤖 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/tools/api-diff/src/Discovery.ts` around lines 140 - 141, Update the declarationRoot assignment in the Discovery configuration to remove the redundant packed conditional and use the intended declaration directory consistently. If non-packed builds should use a different location, correct that branch instead; leave the root assignment unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/tools/api-diff/src/Discovery.ts`:
- Around line 140-141: Update the declarationRoot assignment in the Discovery
configuration to remove the redundant packed conditional and use the intended
declaration directory consistently. If non-packed builds should use a different
location, correct that branch instead; leave the root assignment unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 96f47929-6d66-4e32-b49c-19b3b74af0e0
📒 Files selected for processing (21)
.changeset/detect-api-removals.md.changeset/migrate-api-diff-cli.mdpackages/tools/api-diff/README.mdpackages/tools/api-diff/src/ApiDiff.tspackages/tools/api-diff/src/Cli.tspackages/tools/api-diff/src/Diff.tspackages/tools/api-diff/src/Discovery.tspackages/tools/api-diff/src/Error.tspackages/tools/api-diff/src/Json.tspackages/tools/api-diff/src/Mapping.tspackages/tools/api-diff/src/Model.tspackages/tools/api-diff/src/Report.tspackages/tools/api-diff/src/Snapshot.tspackages/tools/api-diff/src/Worktrees.tspackages/tools/api-diff/src/bin.tspackages/tools/api-diff/test/Cli.test.tspackages/tools/api-diff/test/Diff.test.tspackages/tools/api-diff/test/Discovery.test.tspackages/tools/api-diff/test/Mapping.test.tspackages/tools/api-diff/test/Snapshot.test.tspackages/tools/api-diff/test/utils.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- .changeset/migrate-api-diff-cli.md
- packages/tools/api-diff/src/bin.ts
- packages/tools/api-diff/test/Cli.test.ts
- packages/tools/api-diff/test/utils.ts
- packages/tools/api-diff/test/Snapshot.test.ts
- packages/tools/api-diff/test/Discovery.test.ts
- packages/tools/api-diff/README.md
- packages/tools/api-diff/src/Report.ts
- packages/tools/api-diff/test/Diff.test.ts
- packages/tools/api-diff/test/Mapping.test.ts
- packages/tools/api-diff/src/Model.ts
- Discover public entrypoints independently for each revision - Detect module additions, removals, and likely API moves from snapshots
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tools/api-diff/src/ApiDiff.ts (1)
93-97: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmpty-report guard is unreachable and runs after files are already written.
renderMarkdownReport(Report.ts) always emits header/summary lines unconditionally, soreport.length === 0can never be true — this safety net never fires. Additionally, the check currently runs afterreport.mdand the snapshot/diff JSON files have already been written, so even a hypothetical failure here would leave partial output on disk.♻️ Proposed reordering
+ if (report.length === 0) { + return yield* new ApiDiffError({ message: "Generated Markdown report is empty" }) + } yield* fs.makeDirectory(output, { recursive: true }) yield* fs.writeFileString(path.join(output, "base.snapshot.json"), prettyJson(base)) yield* fs.writeFileString(path.join(output, "head.snapshot.json"), prettyJson(head)) yield* fs.writeFileString(path.join(output, "diff.json"), prettyJson(diff)) yield* fs.writeFileString(path.join(output, "report.md"), report) yield* Console.log(`Wrote ${path.relative(repoRoot, output)} (${diff.changes.length} changes)`) - if (report.length === 0) { - return yield* new ApiDiffError({ message: "Generated Markdown report is empty" }) - }Note this only fixes ordering; since
renderMarkdownReportcan't currently produce an empty string, consider whether this guard is still meaningful as written.🤖 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/tools/api-diff/src/ApiDiff.ts` around lines 93 - 97, Move the report validation in the ApiDiff execution flow before writing report.md or any snapshot/diff JSON files, so a failed validation cannot leave partial output. Reassess the report.length === 0 guard alongside renderMarkdownReport and retain or adjust it only if it checks a meaningful failure condition; preserve the existing ApiDiffError behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tools/api-diff/src/Report.ts`:
- Around line 63-68: Update the moduleCounts grouping in Report.ts to fall back
to the module/package name carried in change.delta when headApiId and baseApiId
are absent, especially for package-added, package-removed, module-added, and
module-removed changes produced by moduleChanges. Preserve the existing
API-ID-derived module logic and use the generic "<package>" bucket only when
neither source provides a name.
---
Outside diff comments:
In `@packages/tools/api-diff/src/ApiDiff.ts`:
- Around line 93-97: Move the report validation in the ApiDiff execution flow
before writing report.md or any snapshot/diff JSON files, so a failed validation
cannot leave partial output. Reassess the report.length === 0 guard alongside
renderMarkdownReport and retain or adjust it only if it checks a meaningful
failure condition; preserve the existing ApiDiffError behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 49e213ce-233f-4ee6-a530-a8b7c1432a6c
📒 Files selected for processing (14)
.changeset/detect-api-removals.mdpackages/tools/api-diff/README.mdpackages/tools/api-diff/src/ApiDiff.tspackages/tools/api-diff/src/Cli.tspackages/tools/api-diff/src/Diff.tspackages/tools/api-diff/src/Discovery.tspackages/tools/api-diff/src/Model.tspackages/tools/api-diff/src/Report.tspackages/tools/api-diff/src/Snapshot.tspackages/tools/api-diff/src/Worktrees.tspackages/tools/api-diff/test/Cli.test.tspackages/tools/api-diff/test/Diff.test.tspackages/tools/api-diff/test/Discovery.test.tsplan.md
💤 Files with no reviewable changes (1)
- packages/tools/api-diff/src/Model.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- .changeset/detect-api-removals.md
- packages/tools/api-diff/test/Discovery.test.ts
- packages/tools/api-diff/README.md
- packages/tools/api-diff/src/Discovery.ts
- packages/tools/api-diff/src/Snapshot.ts
- packages/tools/api-diff/src/Worktrees.ts
| const moduleCounts = new Map<string, number>() | ||
| for (const change of diff.changes) { | ||
| const id = change.headApiId ?? change.baseApiId | ||
| const module = id?.split("#")[0] ?? "<package>" | ||
| moduleCounts.set(module, (moduleCounts.get(module) ?? 0) + 1) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Module/package-level changes collapse into a generic "<package>" bucket.
moduleCounts only derives the module from change.headApiId ?? change.baseApiId. But package-added/package-removed/module-added/module-removed changes (produced by moduleChanges in Diff.ts) never set baseApiId/headApiId — only delta. As a result, every such change is grouped under the literal "<package>" row in the "Changes by module" table instead of the actual module/package name, even though that name is available in delta.
🐛 Proposed fix to fall back to `delta`
const moduleCounts = new Map<string, number>()
for (const change of diff.changes) {
const id = change.headApiId ?? change.baseApiId
- const module = id?.split("#")[0] ?? "<package>"
+ const delta = change.delta as { packageName?: string; from?: string; to?: ReadonlyArray<string> } | undefined
+ const module = id?.split("#")[0] ?? delta?.packageName ?? delta?.from ?? delta?.to?.[0] ?? "<package>"
moduleCounts.set(module, (moduleCounts.get(module) ?? 0) + 1)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const moduleCounts = new Map<string, number>() | |
| for (const change of diff.changes) { | |
| const id = change.headApiId ?? change.baseApiId | |
| const module = id?.split("#")[0] ?? "<package>" | |
| moduleCounts.set(module, (moduleCounts.get(module) ?? 0) + 1) | |
| } | |
| const moduleCounts = new Map<string, number>() | |
| for (const change of diff.changes) { | |
| const id = change.headApiId ?? change.baseApiId | |
| const delta = change.delta as { packageName?: string; from?: string; to?: ReadonlyArray<string> } | undefined | |
| const module = id?.split("#")[0] ?? delta?.packageName ?? delta?.from ?? delta?.to?.[0] ?? "<package>" | |
| moduleCounts.set(module, (moduleCounts.get(module) ?? 0) + 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/tools/api-diff/src/Report.ts` around lines 63 - 68, Update the
moduleCounts grouping in Report.ts to fall back to the module/package name
carried in change.delta when headApiId and baseApiId are absent, especially for
package-added, package-removed, module-added, and module-removed changes
produced by moduleChanges. Preserve the existing API-ID-derived module logic and
use the generic "<package>" bucket only when neither source provides a name.
1 participant
Summary
Testing
Summary by CodeRabbit