Uh oh!
There was an error while loading. Please reload this page.
ci(repo): extend break-check coverage to all SDK packages - #8691
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
@clerk/astro@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/expo@clerk/expo-passkeys@clerk/express@clerk/fastify@clerk/hono@clerk/localizations@clerk/nextjs@clerk/nuxt@clerk/react@clerk/react-router@clerk/shared@clerk/tanstack-react-start@clerk/testing@clerk/ui@clerk/upgrade@clerk/vuecommit: |
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 expands API-change detection to many more packages by updating snapi.config.json and the api-changes workflow, rewrites the workflow’s report-posting step to use actions/github-script, and changes cache key derivation for snapi baselines. It also adds per-package tsconfig.declarations.json files and build:declarations scripts across multiple packages, updates package build scripts (astro/vue/testing/tsup) to run the declaration flow, and adds explicit typeof annotations to two Nuxt component exports. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/api-changes.yml (1)
205-205:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate hardcoded message to reflect expanded package coverage.
The hardcoded message lists only the original 6 packages, but the workflow now monitors 19 packages. This creates confusion about the actual scope of API-change detection.
💬 Proposed fix to list all monitored packages
- echo '**Snapi**: no API changes detected in `@clerk/backend`, `@clerk/clerk-js`, `@clerk/nextjs`, `@clerk/react`, `@clerk/shared`, `@clerk/ui`.'+ echo '**Snapi**: no API changes detected in `@clerk/astro`, `@clerk/backend`, `@clerk/chrome-extension`, `@clerk/clerk-js`, `@clerk/expo`, `@clerk/expo-passkeys`, `@clerk/express`, `@clerk/fastify`, `@clerk/hono`, `@clerk/localizations`, `@clerk/nextjs`, `@clerk/nuxt`, `@clerk/react`, `@clerk/react-router`, `@clerk/shared`, `@clerk/tanstack-react-start`, `@clerk/testing`, `@clerk/ui`, `@clerk/vue`.'Alternatively, consider making this message dynamic based on the snapi.config.json packages list to avoid future mismatches.
🤖 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 @.github/workflows/api-changes.yml at line 205, The hardcoded echo message in the api-changes workflow (the line that runs echo '**Snapi**: no API changes detected...') lists only six packages but the workflow actually monitors 19; update that string to enumerate all 19 monitored packages or (better) make the message dynamic by reading the package list from snapi.config.json so it always reflects the actual snapi package set; locate the echo command (the exact shell command containing '**Snapi**: no API changes detected in') and either expand the list to include every monitored package name or replace it with logic that builds the package list from snapi.config.json before echoing.
🧹 Nitpick comments (2)
packages/astro/package.json (1)
80-81: ⚡ Quick winAdd a lightweight CI test for declaration build wiring.
Since this PR changes package build/declaration scripts and no tests were added, please add a small CI assertion (or workspace script check) that
build:declarationsexists and runs for touched packages.As per coding guidelines: "**/*: If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes."
🤖 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/astro/package.json` around lines 80 - 81, Add a lightweight CI check that validates the package script "build:declarations" exists and runs successfully for packages touched by the PR: implement a new workspace script or CI job that (1) detects changed packages from the commit/PR, (2) for each changed package reads its package.json and asserts the "build:declarations" script key exists (e.g., "build:declarations": "tsc -p tsconfig.declarations.json"), and (3) runs that script (or runs tsc -p tsconfig.declarations.json directly) to ensure declaration wiring succeeds; update CI workflow to call this check for changed packages so future PRs must include or update declaration scripts when build changes are made.packages/localizations/tsconfig.declarations.json (1)
1-8: 💤 Low valueConsider standardizing compiler options across declaration configs.
This config uses
declarationDirand relies on the base tsconfig.json for options likedeclaration,emitDeclarationOnly, andnoEmit. Other packages in this PR (e.g.,packages/nuxt/tsconfig.declarations.json) explicitly set these options and useoutDirinstead. While both approaches work, explicit configuration improves maintainability and makes the intent clearer.📋 Suggestion for explicit configuration
{ "extends": "./tsconfig.json", "compilerOptions": { - "declarationDir": "./dist",- "skipLibCheck": true+ "declaration": true,+ "declarationMap": true,+ "emitDeclarationOnly": true,+ "noEmit": false,+ "outDir": "./dist",+ "skipLibCheck": true }, "exclude": ["node_modules", "tmp", "dist", "**/__tests__/**/*"] }🤖 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/localizations/tsconfig.declarations.json` around lines 1 - 8, This declaration tsconfig currently only sets "declarationDir" and inherits other flags from the base, so make the file explicit and consistent with other packages by adding compilerOptions: set "declaration": true, "emitDeclarationOnly": true, and explicitly set "outDir": "./dist" (and adjust/remove "declarationDir" if you prefer outDir semantics), ensure "noEmit" is not blocking (set to false or omit if base sets true), and keep "skipLibCheck": true; update the JSON in this tsconfig to explicitly declare these keys so the intent and behavior match packages/nuxt's tsconfig.declarations.json.
🤖 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 @.github/workflows/api-changes.yml:
- Line 205: The hardcoded echo message in the api-changes workflow (the line
that runs echo '**Snapi**: no API changes detected...') lists only six packages
but the workflow actually monitors 19; update that string to enumerate all 19
monitored packages or (better) make the message dynamic by reading the package
list from snapi.config.json so it always reflects the actual snapi package set;
locate the echo command (the exact shell command containing '**Snapi**: no API
changes detected in') and either expand the list to include every monitored
package name or replace it with logic that builds the package list from
snapi.config.json before echoing.
---
Nitpick comments:
In `@packages/astro/package.json`:
- Around line 80-81: Add a lightweight CI check that validates the package
script "build:declarations" exists and runs successfully for packages touched by
the PR: implement a new workspace script or CI job that (1) detects changed
packages from the commit/PR, (2) for each changed package reads its package.json
and asserts the "build:declarations" script key exists (e.g.,
"build:declarations": "tsc -p tsconfig.declarations.json"), and (3) runs that
script (or runs tsc -p tsconfig.declarations.json directly) to ensure
declaration wiring succeeds; update CI workflow to call this check for changed
packages so future PRs must include or update declaration scripts when build
changes are made.
In `@packages/localizations/tsconfig.declarations.json`:
- Around line 1-8: This declaration tsconfig currently only sets
"declarationDir" and inherits other flags from the base, so make the file
explicit and consistent with other packages by adding compilerOptions: set
"declaration": true, "emitDeclarationOnly": true, and explicitly set "outDir":
"./dist" (and adjust/remove "declarationDir" if you prefer outDir semantics),
ensure "noEmit" is not blocking (set to false or omit if base sets true), and
keep "skipLibCheck": true; update the JSON in this tsconfig to explicitly
declare these keys so the intent and behavior match packages/nuxt's
tsconfig.declarations.json.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: baf3d4f9-d55d-4bce-92e9-55c2225d2019
📒 Files selected for processing (24)
.changeset/snapi-coverage-all-packages.md.github/workflows/api-changes.ymlpackages/astro/package.jsonpackages/astro/tsconfig.declarations.jsonpackages/express/package.jsonpackages/express/tsconfig.declarations.jsonpackages/fastify/package.jsonpackages/fastify/tsconfig.declarations.jsonpackages/hono/package.jsonpackages/hono/tsconfig.declarations.jsonpackages/localizations/package.jsonpackages/localizations/tsconfig.declarations.jsonpackages/nuxt/package.jsonpackages/nuxt/src/runtime/components/uiComponents.tspackages/nuxt/tsconfig.declarations.jsonpackages/react-router/package.jsonpackages/react-router/tsconfig.declarations.jsonpackages/tanstack-react-start/tsconfig.declarations.jsonpackages/testing/package.jsonpackages/testing/tsconfig.declarations.jsonpackages/testing/tsup.config.tspackages/vue/package.jsonpackages/vue/tsconfig.declarations.jsonsnapi.config.json
Actionable comments posted: 0 |
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)
.github/workflows/api-changes.yml (1)
204-205:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the "no changes" message to reflect all tracked packages.
The message lists only 6 packages (
@clerk/backend,@clerk/clerk-js,@clerk/nextjs,@clerk/react,@clerk/shared,@clerk/ui), butSNAPI_FILTERSnow includes 19 packages. This inconsistency could mislead PR authors about the scope of API-change detection.📝 Proposed fix to include all tracked packages
- echo '**Snapi**: no API changes detected in `@clerk/backend`, `@clerk/clerk-js`, `@clerk/nextjs`, `@clerk/react`, `@clerk/shared`, `@clerk/ui`.'+ echo '**Snapi**: no API changes detected in `@clerk/astro`, `@clerk/backend`, `@clerk/chrome-extension`, `@clerk/clerk-js`, `@clerk/expo`, `@clerk/expo-passkeys`, `@clerk/express`, `@clerk/fastify`, `@clerk/hono`, `@clerk/localizations`, `@clerk/nextjs`, `@clerk/nuxt`, `@clerk/react`, `@clerk/react-router`, `@clerk/shared`, `@clerk/tanstack-react-start`, `@clerk/testing`, `@clerk/ui`, `@clerk/vue`.'🤖 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 @.github/workflows/api-changes.yml around lines 204 - 205, The "no API changes" echo is out-of-date: replace the hardcoded six-package list with the full set of tracked packages (or reference the SNAPI_FILTERS variable) so the message matches SNAPI_FILTERS; locate the check that greps api-changes-report.md and update the echoed string to enumerate all 19 package names (or dynamically expand SNAPI_FILTERS) so PR authors see an accurate list of packages covered by API-change detection.
🤖 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 @.github/workflows/api-changes.yml:
- Around line 204-205: The "no API changes" echo is out-of-date: replace the
hardcoded six-package list with the full set of tracked packages (or reference
the SNAPI_FILTERS variable) so the message matches SNAPI_FILTERS; locate the
check that greps api-changes-report.md and update the echoed string to enumerate
all 19 package names (or dynamically expand SNAPI_FILTERS) so PR authors see an
accurate list of packages covered by API-change detection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 42d0ad54-4bb5-4675-883c-9f32d2168f9d
📒 Files selected for processing (1)
.github/workflows/api-changes.yml
…al declaration builds
The snapi tool was renamed to break-check (package @clerk/break-check, binary break-check, repo clerk/break-check). Point the API Changes workflow at the new pkg.pr.new build, rename the config file and all SNAPI_* env vars to BREAK_CHECK_*, and update cache keys, worktree paths, and the PR-comment marker. The deprecated snapi.config.json fallback and the old secret mapping are dropped; the workflow reads the BREAK_CHECK_ANTHROPIC_API_KEY secret directly.
Break Check: no API changes detected across the tracked packages. |
The check-api checkout resolved refs/pull/N/merge by default, so the "current" side floated with the base branch tip and break-check attributed unrelated base-branch changes to the PR (clerk/break-check#32). Pin the checkout to pull_request.head.sha; the baseline is already pinned to base.sha.
The baseline-seeding guard must skip seeding when the base ref already tracks coverage under either config name. A base that predates this rename still carries snapi.config.json, so the break-check-only check treated it as untracked and force-fit the full 19-package config onto it. That diffed newly-tracked packages (astro, nuxt, react-router) against their bundled .d.ts and produced phantom breaking changes. This reads the base's real coverage and goes no-op once main carries break-check.config.json.
The build:dts -> build:declarations rename updated package.json and the build script's inline --onSuccess, but left the stale build:dts reference in the tsup config onSuccess. pnpm build masks it via the CLI override; pnpm dev / dev:pub run the config onSuccess and hit a missing script.
| export const UserProfile: typeof BaseUserProfile = Object.assign(_UserProfile, { | ||
| Page: BaseUserProfile.Page, | ||
| Link: BaseUserProfile.Link, | ||
| }); | ||
| const _OrganizationProfile = wrapComponentWithRouting(BaseOrganizationProfile, 'OrganizationProfile'); | ||
| export const OrganizationProfile = Object.assign(_OrganizationProfile, { | ||
| export const OrganizationProfile: typeof BaseOrganizationProfile = Object.assign(_OrganizationProfile, { |
Extends break-check API-change tracking from the original 6 packages to all 19 publishable SDKs (skipping
@clerk/upgrade,@clerk/dev-cli, and@clerk/msw, which either have no public JS API or are private).Most of the work is mechanical: each newly-tracked package gets a 3-line
tsconfig.declarations.jsonand a"build:declarations": "tsc -p tsconfig.declarations.json"script, mirroring the pattern already inbackend,clerk-js,nextjs,react,shared, andui.@clerk/vueusesvue-tscinstead oftscso SFCs resolve correctly. The choice to standardize on a TypeScript-compiler-emitted per-file.d.tstree (vs tsup's bundled.d.ts) is deliberate: per-file output is what API Extractor / break-check can analyze without inventing duplicate-index-signature errors.Two non-mechanical bits worth a look:
packages/nuxt/src/runtime/components/uiComponents.tsadds explicittypeof BaseUserProfile/typeof BaseOrganizationProfileannotations. Without them tsc emits TS2742 ("inferred type cannot be named without a reference to '../../../node_modules/@clerk/vue/dist/types'") becauseObject.assign(component, { Page, Link })produces a structural type that can only be written by reaching into a transitive dep. Same emitted type, just spelled out.packages/tanstack-react-start/tsconfig.declarations.jsonadds anexcludefor__tests__/*.test.{ts,tsx}. The pre-existing config didn't exclude tests, so itsbuild:declarationswas actually failing on a test file's type assertion. It just hadn't been wired into the workflow yet, so nobody noticed.break-check.config.jsonand.github/workflows/api-changes.ymlare expanded to cover the new packages in both the path filter andBREAK_CHECK_FILTERS. Verified locally:pnpm turbo build:declarationsacross all 19 packages runs clean (38 tasks, ~57s).The tool itself was also renamed upstream from snapi to break-check (package
@clerk/break-check, binarybreak-check), so this branch moves with it: the workflow points at the new pkg.pr.new build,snapi.config.jsonbecomesbreak-check.config.json, and theSNAPI_*env/secret names becomeBREAK_CHECK_*. No backwards-compat shims left behind.