feat(astro): Remove createRouteMatcher and support Astro 7#8974
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 2cb9fa4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughUpdates Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@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/vue
commit: |
There was a problem hiding this comment.
dont need tailwind in this...
API Changes Report
Summary
🔴 Breaking changes index (1)Every breaking change, up front. Full diffs are in the package sections below.
@clerk/astroCurrent version: 3.4.20 Subpath
|
There was a problem hiding this comment.
the change basically just removed the early return; in favor of wrapping the menu-item registration logic in the existing else branch
There was a problem hiding this comment.
Astro v6 compatibility smoke test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
integration/tests/astro/middleware.test.ts (1)
183-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid a permanent unconditional skip for this regression case.
test.skip(true, ...)disables this check in every run, so routing/auth regressions here can silently reappear. Prefer a trackedtest.fixme(with issue ID) or a version-gated skip so coverage returns automatically when the matcher behavior is fixed.🤖 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 `@integration/tests/astro/middleware.test.ts` around lines 183 - 186, The test.skip(true, ...) call unconditionally disables this regression test in every run, which allows routing and auth regressions to silently reappear. Replace this with test.fixme(...) that includes a tracked issue ID reference, or implement version-gated conditional skip logic based on the Astro version so the test coverage automatically resumes when the routing normalization issue is resolved. The key is to make the skip temporary and tracked rather than permanent.
🤖 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 `@integration/tests/astro/middleware.test.ts`:
- Around line 183-186: The test.skip(true, ...) call unconditionally disables
this regression test in every run, which allows routing and auth regressions to
silently reappear. Replace this with test.fixme(...) that includes a tracked
issue ID reference, or implement version-gated conditional skip logic based on
the Astro version so the test coverage automatically resumes when the routing
normalization issue is resolved. The key is to make the skip temporary and
tracked rather than permanent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b4b2e9c-0535-4606-8b56-033870a8b624
📒 Files selected for processing (4)
integration/tests/astro/compatibility.test.tsintegration/tests/astro/middleware.test.tspackages/astro/package.jsonpackages/astro/src/astro-components/interactive/UserButton/MenuItemRenderer.astro
🚧 Files skipped from review as they are similar to previous changes (3)
- integration/tests/astro/compatibility.test.ts
- packages/astro/package.json
- packages/astro/src/astro-components/interactive/UserButton/MenuItemRenderer.astro
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 `@integration/testUtils/machineAuthHelpers.ts`:
- Around line 275-281: In the POST request handling blocks, the status assertion
must be moved before the JSON parsing. Currently, the code calls
postWithSessionRes.json() before checking postWithSessionRes.status(), which
means if the response is an error, the json() call may fail before the status
assertion runs, obscuring the actual failure. Reorder the statements so that
expect(postWithSessionRes.status()).toBe(200) is called immediately after the
post request completes, before calling await postWithSessionRes.json(). Apply
this same fix to both the postWithSessionRes block and the similar block
referenced in the "Also applies to" comment.
🪄 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 YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: af4cda56-7137-4065-a8e4-43d80f4d8263
📒 Files selected for processing (1)
integration/testUtils/machineAuthHelpers.ts
| const postWithSessionRes = await u.page.request.post(url, { | ||
| headers: { Origin: origin }, | ||
| }); | ||
| const sessionData = await postWithSessionRes.json(); | ||
| expect(postWithSessionRes.status()).toBe(200); | ||
| expect(sessionData.userId).toBe(fakeBapiUser.id); | ||
| expect(sessionData.tokenType).toBe(TokenType.SessionToken); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Assert response status before parsing JSON bodies in POST branches.
json() can throw on non-JSON error responses before the status assertion runs, which obscures the real failure. Assert status first, then parse.
Suggested patch
const postWithSessionRes = await u.page.request.post(url, {
headers: { Origin: origin },
});
- const sessionData = await postWithSessionRes.json();
expect(postWithSessionRes.status()).toBe(200);
+ const sessionData = await postWithSessionRes.json();
expect(sessionData.userId).toBe(fakeBapiUser.id);
expect(sessionData.tokenType).toBe(TokenType.SessionToken);
const postWithApiKeyRes = await u.page.request.post(url, {
headers: { Authorization: `Bearer ${fakeAPIKey.secret}`, Origin: origin },
});
- const apiKeyData = await postWithApiKeyRes.json();
expect(postWithApiKeyRes.status()).toBe(200);
+ const apiKeyData = await postWithApiKeyRes.json();
expect(apiKeyData.userId).toBe(fakeBapiUser.id);
expect(apiKeyData.tokenType).toBe(TokenType.ApiKey);Also applies to: 283-285
🤖 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 `@integration/testUtils/machineAuthHelpers.ts` around lines 275 - 281, In the
POST request handling blocks, the status assertion must be moved before the JSON
parsing. Currently, the code calls postWithSessionRes.json() before checking
postWithSessionRes.status(), which means if the response is an error, the json()
call may fail before the status assertion runs, obscuring the actual failure.
Reorder the statements so that expect(postWithSessionRes.status()).toBe(200) is
called immediately after the post request completes, before calling await
postWithSessionRes.json(). Apply this same fix to both the postWithSessionRes
block and the similar block referenced in the "Also applies to" comment.
|
!allow-major |
wobsoriano
changed the title
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 `@integration/templates/astro-node/src/pages/discover.astro`:
- Around line 5-9: Add an end-to-end signed-out navigation test in the existing
Astro component test suite for `/discover`, asserting it redirects to the
sign-in route. Reuse the test’s established navigation and redirect assertion
patterns, and ensure the test covers the guard introduced in the `/discover`
page.
🪄 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 YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 99030421-dd59-4675-b8fc-a285043b53d5
📒 Files selected for processing (12)
.changeset/floppy-buckets-help.md.changeset/major-astro-remove-route-matcher.mdintegration/templates/astro-node/src/middleware.tsintegration/templates/astro-node/src/pages/api/protected/current-org.tsintegration/templates/astro-node/src/pages/api/protected/only-admin.tsintegration/templates/astro-node/src/pages/discover.astrointegration/templates/astro-node/src/pages/user.astrointegration/tests/astro/components.test.tsintegration/tests/astro/middleware.test.tspackages/astro/package.jsonpackages/astro/src/server/index.tspackages/astro/src/server/route-matcher.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)
💤 Files with no reviewable changes (2)
- packages/astro/src/server/route-matcher.ts
- packages/astro/src/server/index.ts
4 participants
Description
This PR does 3 things:
createRouteMatcherDocs with migration instructions is ready https://github.com/clerk/clerk/pull/2988
Fixes #9165
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change