clerk auth#37
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Clerk-hosted login/register/callback/logout across Next/Nuxt/SvelteKit; wires a redirect-response interrupt into the auth runtime and adapters; refactors auth-clerk types/implementation and tests; scaffolds provider routes via the CLI; updates configs/docs; and converts many package manifests to use catalog: with publish/validation tooling. ChangesClerk hosted auth + runtime plumbing
Catalog policy & manifest changes
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
apps/blog-nuxt/.env.example (1)
49-54: 💤 Low valueConsider alphabetical ordering for Clerk environment variables.
The static analysis tool suggests reordering the Clerk variables alphabetically. While this doesn't affect functionality, consistent alphabetical ordering improves maintainability and makes it easier to locate variables in larger env files.
Suggested ordering
CLERK_PUBLISHABLE_KEY= -CLERK_SECRET_KEY= CLERK_API_URL= CLERK_FRONTEND_API= CLERK_REDIRECT_URI= +CLERK_SECRET_KEY= CLERK_SESSION_COOKIE=🤖 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 `@apps/blog-nuxt/.env.example` around lines 49 - 54, The Clerk env variables in the .env example are not in alphabetical order; reorder the CLERK_* entries so they follow alphabetical order (e.g., CLERK_API_URL, CLERK_FRONTEND_API, CLERK_PUBLISHABLE_KEY, CLERK_REDIRECT_URI, CLERK_SECRET_KEY, CLERK_SESSION_COOKIE) to improve maintainability and consistency when locating variables.packages/config/src/defaults.ts (2)
1574-1576: ⚡ Quick winConsider strengthening the type guard check.
The
isClerkProviderConfigtype guard only checks if the value is a non-null, non-array object. This could incorrectly accept plain objects or other types. Consider adding a check for expected properties likesecretKeyorpublishableKeyto make the guard more robust.🛡️ Suggested refinement
function isClerkProviderConfig(value: unknown): value is AuthClerkProviderConfig { - return Boolean(value) && typeof value === 'object' && !Array.isArray(value) + return Boolean(value) + && typeof value === 'object' + && !Array.isArray(value) + && ('secretKey' in value || 'publishableKey' in value || 'redirectUri' in value) }🤖 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/config/src/defaults.ts` around lines 1574 - 1576, The isClerkProviderConfig type guard is too loose (only checks non-null object); tighten it by verifying expected AuthClerkProviderConfig shape—inside isClerkProviderConfig check for required string properties (e.g., "secretKey" and/or "publishableKey", and optionally "apiUrl" or "domain") and ensure they are strings (and non-empty if desired) so the guard only returns true for objects matching the AuthClerkProviderConfig structure.
1564-1564: 💤 Low valueConsider adding URL format validation for
redirectUriat the config level.The
redirectUriis trimmed but not validated to be a well-formed URL. While downstream code (e.g., WorkOS validates presence, Clerk's URL constructor validates format in a try-catch), these checks are scattered across auth implementations and happen at runtime. Validating at the config defaults level would catch misconfigurations earlier and provide consistent error messaging.🤖 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/config/src/defaults.ts` at line 1564, The redirectUri value is only trimmed but not validated; update the defaults logic that sets redirectUri (currently using config.redirectUri?.trim() || undefined) to validate the trimmed value with the URL constructor (or a safe URL-parse helper) and throw a clear configuration error if it is not a well-formed URL; specifically, compute const trimmed = config.redirectUri?.trim(); if (trimmed) { try { new URL(trimmed); redirectUri = trimmed; } catch (e) { throw new Error(`Invalid redirectUri in config: ${trimmed}`); } } else { redirectUri = undefined; } This ensures redirectUri is validated at config resolution (referencing redirectUri and config.redirectUri?.trim()).packages/cli/tests/cli.test.ts (1)
2100-2118: ⚡ Quick winAdd integration assertion for
.env.exampleClerk redirect keyThis block verifies
.envbut not.env.example, even thoughupdatedEnvExample: trueis part of the expected result. Add a direct assertion to prevent regressions in example env scaffolding.Suggested patch
const rerunEnv = await readFile(join(projectRoot, '.env'), 'utf8') + const rerunEnvExample = await readFile(join(projectRoot, '.env.example'), 'utf8') expect(rerunEnv).toContain('AUTH_GOOGLE_CLIENT_ID=') expect(rerunEnv).toContain('WORKOS_CLIENT_ID=') expect(rerunEnv).toContain('CLERK_PUBLISHABLE_KEY=') expect(rerunEnv).toContain('CLERK_REDIRECT_URI=') + expect(rerunEnvExample).toContain('CLERK_REDIRECT_URI=')🤖 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/cli/tests/cli.test.ts` around lines 2100 - 2118, The test currently asserts that the generated .env contains CLERK_REDIRECT_URI but misses asserting the same key in .env.example even though updatedEnvExample is expected true; update the test around projectInternals.installAuthIntoProject/ readFile(join(projectRoot, '.env')) to also read join(projectRoot, '.env.example') and add an assertion that the example contains 'CLERK_REDIRECT_URI=' (similar to the existing .env assertion) so the example env scaffolding is explicitly validated.scripts/publish-with-resolved-catalogs.test.mjs (1)
33-90: ⚡ Quick winAdd a unit test for unresolved
catalog:entries.
resolveCatalogRangesInManifestthrowsCannot resolve catalog range for ${section}.${pkg}.when acatalog:value is missing from the root catalog (a fairly easy footgun when a new package is added without updating the root catalog). A small regression test would lock in this behavior:🧪 Suggested additional test
+test('catalog resolver throws when a catalog entry is missing', () => { + assert.throws( + () => resolveCatalogRangesInManifest({ + dependencies: { '@holo-js/missing': 'catalog:' }, + }, {}), + /Cannot resolve catalog range for dependencies\.@holo-js\/missing/, + ) +})🤖 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 `@scripts/publish-with-resolved-catalogs.test.mjs` around lines 33 - 90, Add a unit test that asserts resolveCatalogRangesInManifest throws the expected error when a manifest contains a "catalog:" dependency that is not present in the root catalog: call resolveCatalogRangesInManifest with a manifest whose dependencies (or peerDependencies) include '@holo-js/newpkg': 'catalog:' and a root catalog object that omits '@holo-js/newpkg', and assert it rejects/throws with the message /Cannot resolve catalog range for .*@holo-js\/newpkg/ (or the exact message format used by resolveCatalogRangesInManifest) to lock in the current error behavior.scripts/publish-with-resolved-catalogs.mjs (1)
22-29: 💤 Low valueFilter
packages/*entries that don't contain apackage.json.
listPackageManifestPathsreturnspackages/<dir>/package.jsonfor every subdirectory, regardless of whether the manifest exists. A non-package directory (scratch folder, lingering output dir, or fresh clone artifact) will make the subsequentreadFilethrow mid-rewrite, sending the script straight into thefinallyblock with only some manifests captured for restore. Filter to entries that actually contain apackage.jsonto make the script resilient to stray directories.🛡️ Suggested guard
-import { readdir, readFile, writeFile } from 'node:fs/promises' +import { readdir, readFile, stat, writeFile } from 'node:fs/promises' @@ - return entries - .filter(entry => entry.isDirectory()) - .map(entry => join(packageDirectory, entry.name, 'package.json')) + const candidatePaths = entries + .filter(entry => entry.isDirectory()) + .map(entry => join(packageDirectory, entry.name, 'package.json')) + + const existing = await Promise.all(candidatePaths.map(async (manifestPath) => { + try { + const info = await stat(manifestPath) + return info.isFile() ? manifestPath : undefined + } + catch { + return undefined + } + })) + + return existing.filter((manifestPath) => typeof manifestPath === 'string')🤖 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 `@scripts/publish-with-resolved-catalogs.mjs` around lines 22 - 29, listPackageManifestPaths currently returns packages/<dir>/package.json for every subdirectory without verifying the file exists; change it to filter out dirs that lack a package.json by checking the existence/readability of join(packageDirectory, entry.name, 'package.json') (use an async fs check such as fs.promises.access or fs.promises.stat) and only include paths where that check succeeds—update the return pipeline in listPackageManifestPaths to await or Promise.all the existence checks and return only actual manifest paths.apps/docs/docs/auth/clerk.md (1)
5-8: 💤 Low valueShow the install command.
The "Configuration" intro says "Install Clerk auth and enable the Clerk Account Portal in the Clerk Dashboard." but does not show the CLI command. Adding the exact command (e.g.,
holo install auth --clerk) avoids ambiguity, especially given the new CLI hosted-auth route scaffolding paths this PR adds.🤖 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 `@apps/docs/docs/auth/clerk.md` around lines 5 - 8, Update the "Configuration" section in the clerk.md doc to include the exact CLI install command (add the line with the command: holo install auth --clerk) immediately after the sentence that says "Install Clerk auth and enable the Clerk Account Portal in the Clerk Dashboard."; keep the note about enabling the Clerk Account Portal and, if helpful, mention that this command wires up the new hosted-auth route scaffolding so readers know it sets up the correct paths.package.json (1)
169-169: 💤 Low value
changesetresolution depends on inherited PATH.
node scripts/publish-with-resolved-catalogs.mjsruns the publish script under Node, but the script invokesspawnSync('changeset', …)(see Line 87 inscripts/publish-with-resolved-catalogs.mjs). Resolution relies onbun run releasehaving injectednode_modules/.bininto PATH. That works in CI on Linux/macOS, but if anyone runs the publish script directly vianode scripts/publish-with-resolved-catalogs.mjs, the binary will not be found. Consider either documenting that release must go throughbun run release, or invoking the binary viabun x changeset publish/ a resolved absolute path inside the script.🤖 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 `@package.json` at line 169, The release script relies on an inherited PATH so spawnSync('changeset', ...) in scripts/publish-with-resolved-catalogs.mjs can fail when run via node directly; update either package.json "release" or the script: change the release command to ensure the changeset binary is executed via Bun (e.g., use "bun x changeset publish" or run the script through bun), or modify publish-with-resolved-catalogs.mjs to resolve and call the changeset binary explicitly (resolve the binary from node_modules/.bin or use require.resolve/cross-spawn with an absolute path) so spawnSync refers to a guaranteed executable path instead of relying on inherited PATH.
🤖 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 `@apps/docs/docs/auth/clerk.md`:
- Around line 9-37: Add documentation for AUTH_CLERK_PROVIDER to the env vars
table and note the default; specifically, document the env key
AUTH_CLERK_PROVIDER (optional, default "app") and enumerate valid values that
map to the clerk.provider field used in defineAuthConfig (e.g., "app", "org" or
whatever supported by your Clerk integration), and add a one-line note
clarifying CLERK_SESSION_COOKIE is optional and defaults to "__session" even
though it isn't referenced in the example config.
In `@packages/adapter-sveltekit/src/index.ts`:
- Around line 48-50: The redirectResponse function's status parameter is
currently inferred as number and must be explicitly typed to match the
SvelteKitModule.redirect signature; update the redirectResponse declaration so
its status parameter is typed as the union 301 | 302 | 303 | 307 | 308 (and
ensure any call sites pass one of those literals or are narrowed accordingly) so
that passing status into SvelteKitModule.redirect does not produce a TypeScript
error; locate the redirectResponse function and change its status parameter type
to that literal union to align with redirect and SvelteKitModule.
In `@packages/auth-clerk/src/contracts.ts`:
- Around line 129-133: The ClerkAuthFacade methods currently only accept the DOM
Request type which causes false TS errors for Nuxt/Node-style request-like
inputs; update the method signatures on ClerkAuthFacade (loginWithClerk,
registerWithClerk, logoutWithClerk, completeClerkAuth) to accept a broader input
type such as Request | RequestLike (or a locally defined
RequestLike/fetch-compatible union like Request | RequestInfo | { headers?:
HeadersInit; method?: string; url?: string; body?: any }) and import or declare
RequestLike where needed so the implementations in src/index.ts that normalize
various framework request shapes match the contract.
In `@packages/auth-clerk/src/index.ts`:
- Around line 1483-1486: completeClerkAuth currently ignores the TUserAttributes
generic and always returns Promise<ClerkCompleteAuthResult>, losing the mapped
user type; update the signature to return a generic result (e.g.,
Promise<ClerkCompleteAuthResult<TUserAttributes>>) and propagate TUserAttributes
through any helper types and factory functions that build the success result,
including the options.user mapper/callback handling so the returned user shape
reflects the provided mapper; ensure all internal uses that construct or cast
ClerkCompleteAuthResult are updated to the generic form to preserve typing
across the function.
- Around line 554-556: createClerkReturnUrl currently constructs a URL from
untrusted returnTo allowing open redirects; update createClerkReturnUrl to
validate returnTo first: accept only relative paths (start with '/') or
same-origin URLs by parsing new URL(returnTo, request.url) and checking that
parsed.origin === new URL(request.url).origin and parsed.pathname
startsWith('/'); reject or normalize anything with a different origin, protocol,
host, or suspicious characters and return '/' as a safe default. Use the
function name createClerkReturnUrl and ensure callers like logoutWithClerk
receive only the validated/normalized URL.
In `@packages/cli/src/project/scaffold/framework-renderers.ts`:
- Around line 160-173: The callback templates currently redirect successful auth
to '/admin' which doesn't exist in fresh scaffolds; update
renderNuxtHostedAuthCallbackRoute (and the other two similar templates at the
other ranges) to default the post-auth redirect to '/' instead of '/admin', or
wire a configurable property on HostedAuthProviderSpec (e.g.,
spec.postAuthRedirect) and use that with a fallback to '/' when rendering the
sendRedirect call so new projects land on a valid route.
In `@scripts/publish-with-resolved-catalogs.mjs`:
- Around line 67-81: The restoration in the finally block uses
Promise.all([...originalManifests].map(([manifestPath, contents]) =>
writeFile(manifestPath, contents))) which can reject fast and hide other restore
failures; change this to use Promise.allSettled so every writeFile for each
entry in originalManifests is attempted, then inspect the results and, if any
failed, throw an aggregated Error (or log details) summarizing which
manifestPath writes failed and their reasons; update the finally block around
originalManifests and writeFile and ensure the aggregated error propagates after
allSettled completes.
---
Nitpick comments:
In `@apps/blog-nuxt/.env.example`:
- Around line 49-54: The Clerk env variables in the .env example are not in
alphabetical order; reorder the CLERK_* entries so they follow alphabetical
order (e.g., CLERK_API_URL, CLERK_FRONTEND_API, CLERK_PUBLISHABLE_KEY,
CLERK_REDIRECT_URI, CLERK_SECRET_KEY, CLERK_SESSION_COOKIE) to improve
maintainability and consistency when locating variables.
In `@apps/docs/docs/auth/clerk.md`:
- Around line 5-8: Update the "Configuration" section in the clerk.md doc to
include the exact CLI install command (add the line with the command: holo
install auth --clerk) immediately after the sentence that says "Install Clerk
auth and enable the Clerk Account Portal in the Clerk Dashboard."; keep the note
about enabling the Clerk Account Portal and, if helpful, mention that this
command wires up the new hosted-auth route scaffolding so readers know it sets
up the correct paths.
In `@package.json`:
- Line 169: The release script relies on an inherited PATH so
spawnSync('changeset', ...) in scripts/publish-with-resolved-catalogs.mjs can
fail when run via node directly; update either package.json "release" or the
script: change the release command to ensure the changeset binary is executed
via Bun (e.g., use "bun x changeset publish" or run the script through bun), or
modify publish-with-resolved-catalogs.mjs to resolve and call the changeset
binary explicitly (resolve the binary from node_modules/.bin or use
require.resolve/cross-spawn with an absolute path) so spawnSync refers to a
guaranteed executable path instead of relying on inherited PATH.
In `@packages/cli/tests/cli.test.ts`:
- Around line 2100-2118: The test currently asserts that the generated .env
contains CLERK_REDIRECT_URI but misses asserting the same key in .env.example
even though updatedEnvExample is expected true; update the test around
projectInternals.installAuthIntoProject/ readFile(join(projectRoot, '.env')) to
also read join(projectRoot, '.env.example') and add an assertion that the
example contains 'CLERK_REDIRECT_URI=' (similar to the existing .env assertion)
so the example env scaffolding is explicitly validated.
In `@packages/config/src/defaults.ts`:
- Around line 1574-1576: The isClerkProviderConfig type guard is too loose (only
checks non-null object); tighten it by verifying expected
AuthClerkProviderConfig shape—inside isClerkProviderConfig check for required
string properties (e.g., "secretKey" and/or "publishableKey", and optionally
"apiUrl" or "domain") and ensure they are strings (and non-empty if desired) so
the guard only returns true for objects matching the AuthClerkProviderConfig
structure.
- Line 1564: The redirectUri value is only trimmed but not validated; update the
defaults logic that sets redirectUri (currently using config.redirectUri?.trim()
|| undefined) to validate the trimmed value with the URL constructor (or a safe
URL-parse helper) and throw a clear configuration error if it is not a
well-formed URL; specifically, compute const trimmed =
config.redirectUri?.trim(); if (trimmed) { try { new URL(trimmed); redirectUri =
trimmed; } catch (e) { throw new Error(`Invalid redirectUri in config:
${trimmed}`); } } else { redirectUri = undefined; } This ensures redirectUri is
validated at config resolution (referencing redirectUri and
config.redirectUri?.trim()).
In `@scripts/publish-with-resolved-catalogs.mjs`:
- Around line 22-29: listPackageManifestPaths currently returns
packages/<dir>/package.json for every subdirectory without verifying the file
exists; change it to filter out dirs that lack a package.json by checking the
existence/readability of join(packageDirectory, entry.name, 'package.json') (use
an async fs check such as fs.promises.access or fs.promises.stat) and only
include paths where that check succeeds—update the return pipeline in
listPackageManifestPaths to await or Promise.all the existence checks and return
only actual manifest paths.
In `@scripts/publish-with-resolved-catalogs.test.mjs`:
- Around line 33-90: Add a unit test that asserts resolveCatalogRangesInManifest
throws the expected error when a manifest contains a "catalog:" dependency that
is not present in the root catalog: call resolveCatalogRangesInManifest with a
manifest whose dependencies (or peerDependencies) include '@holo-js/newpkg':
'catalog:' and a root catalog object that omits '@holo-js/newpkg', and assert it
rejects/throws with the message /Cannot resolve catalog range for
.*@holo-js\/newpkg/ (or the exact message format used by
resolveCatalogRangesInManifest) to lock in the current error 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed0b02bf-81b6-41b3-8295-8a839d41914c
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackages/cli/src/generated/workspaceCatalog.tsis excluded by!**/generated/**
📒 Files selected for processing (99)
apps/blog-next/.env.exampleapps/blog-next/app/api/auth/clerk/callback/route.tsapps/blog-next/app/api/auth/clerk/login/route.tsapps/blog-next/app/api/auth/clerk/logout/route.tsapps/blog-next/app/api/auth/clerk/register/route.tsapps/blog-next/app/auth-nav.tsxapps/blog-next/app/login/page.tsxapps/blog-next/app/register/page.tsxapps/blog-next/config/auth.tsapps/blog-nuxt/.env.exampleapps/blog-nuxt/app/app.vueapps/blog-nuxt/app/pages/login/index.vueapps/blog-nuxt/app/pages/register/index.vueapps/blog-nuxt/config/auth.tsapps/blog-nuxt/server/api/auth/clerk/callback.get.tsapps/blog-nuxt/server/api/auth/clerk/login.get.tsapps/blog-nuxt/server/api/auth/clerk/logout.post.tsapps/blog-nuxt/server/api/auth/clerk/register.get.tsapps/blog-sveltekit/.env.exampleapps/blog-sveltekit/config/auth.tsapps/blog-sveltekit/src/routes/+layout.svelteapps/blog-sveltekit/src/routes/api/auth/clerk/callback/+server.tsapps/blog-sveltekit/src/routes/api/auth/clerk/login/+server.tsapps/blog-sveltekit/src/routes/api/auth/clerk/logout/+server.tsapps/blog-sveltekit/src/routes/api/auth/clerk/register/+server.tsapps/blog-sveltekit/src/routes/login/+page.svelteapps/blog-sveltekit/src/routes/register/+page.svelteapps/docs/docs/auth/clerk.mdpackage.jsonpackages/adapter-next/package.jsonpackages/adapter-next/src/runtime.tspackages/adapter-nuxt/package.jsonpackages/adapter-nuxt/src/runtime/composables/index.tspackages/adapter-sveltekit/package.jsonpackages/adapter-sveltekit/src/index.tspackages/auth-clerk/package.jsonpackages/auth-clerk/src/contracts.tspackages/auth-clerk/src/index.tspackages/auth-clerk/src/jwt.tspackages/auth-clerk/tests/package.test.tspackages/auth-social-apple/package.jsonpackages/auth-social-discord/package.jsonpackages/auth-social-facebook/package.jsonpackages/auth-social-github/package.jsonpackages/auth-social-google/package.jsonpackages/auth-social-linkedin/package.jsonpackages/auth-social/package.jsonpackages/auth-workos/package.jsonpackages/auth/package.jsonpackages/auth/src/contracts.tspackages/auth/src/runtime.tspackages/auth/src/runtime/requestAccess.tspackages/auth/src/runtime/responseCookies.tspackages/authorization/package.jsonpackages/broadcast/package.jsonpackages/cache-db/package.jsonpackages/cache-redis/package.jsonpackages/cache/package.jsonpackages/cli/package.jsonpackages/cli/src/project.tspackages/cli/src/project/scaffold.tspackages/cli/src/project/scaffold/config-renderers.tspackages/cli/src/project/scaffold/framework-renderers.tspackages/cli/src/project/scaffold/framework.tspackages/cli/src/project/scaffold/project-renderers.tspackages/cli/tests/cli.test.tspackages/config/package.jsonpackages/config/src/defaults.tspackages/config/src/index.tspackages/config/src/types.tspackages/config/tests/config.test.tspackages/core/package.jsonpackages/core/src/portable/holo.tspackages/create-holo-js/package.jsonpackages/db-mysql/package.jsonpackages/db-postgres/package.jsonpackages/db-sqlite/package.jsonpackages/db/package.jsonpackages/events/package.jsonpackages/flux-react/package.jsonpackages/flux-svelte/package.jsonpackages/flux-vue/package.jsonpackages/flux/package.jsonpackages/forms/package.jsonpackages/mail/package.jsonpackages/media/package.jsonpackages/notifications/package.jsonpackages/queue-db/package.jsonpackages/queue-redis/package.jsonpackages/queue/package.jsonpackages/security/package.jsonpackages/session/package.jsonpackages/storage-s3/package.jsonpackages/storage/package.jsonpackages/validation/package.jsonscripts/publish-with-resolved-catalogs.mjsscripts/publish-with-resolved-catalogs.test.mjsscripts/validate-dependency-version-policy.mjsscripts/validate-dependency-version-policy.test.mjs
💤 Files with no reviewable changes (1)
- packages/auth-clerk/src/jwt.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/adapter-next/src/config.ts`:
- Line 60: The code unconditionally merges
HOLO_OPTIONAL_SERVER_EXTERNAL_PACKAGES into the externalized set (see the
creation of serverExternalPackages/new Set([...HOLO_SERVER_EXTERNAL_PACKAGES,
...HOLO_OPTIONAL_SERVER_EXTERNAL_PACKAGES, ...existingExternal])) which causes
Next/Turbopack to fail when optional packages are not installed; change the
merge to only include optional entries that actually exist in the project (e.g.,
check package.json dependencies/peerDependencies or use
require.resolve/try-catch to test each name) before spreading into the Set, so
HOLO_OPTIONAL_SERVER_EXTERNAL_PACKAGES is filtered at runtime and only installed
packages are added alongside HOLO_SERVER_EXTERNAL_PACKAGES and existingExternal.
In `@packages/auth-clerk/tests/package.test.ts`:
- Around line 143-145: When handling avatar changes in the update path, also
keep avatarUrl in sync: when you check typeof input.avatar !== 'undefined' and
assign user.avatar = input.avatar, also set user.avatarUrl = input.avatar so
serialize() (which prefers user.avatarUrl) reflects updates; apply the same
change to the other update block referenced (lines ~167-175) to avoid stale
avatarUrl after subsequent updates.
- Line 72: Mocks currently treat FetchMockInput by calling String(input), which
converts Request objects to "[object Request]" and breaks URL matching; update
the fetch-mock handling to detect Request instances (using instanceof Request)
and extract the actual URL (request.url) for matching, handle URL values by
using url.href, and fall back to String(input) for plain strings; apply this
change where FetchMockInput is used to determine the request URL in the test
mocks (references: the FetchMockInput type and each mock location that currently
stringifies inputs).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a77d687b-1a9a-4098-8720-8782e10c831a
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackages/cli/src/generated/workspaceCatalog.tsis excluded by!**/generated/**
📒 Files selected for processing (19)
apps/blog-nuxt/.env.exampleapps/docs/docs/auth/clerk.mdpackage.jsonpackages/adapter-next/src/config.tspackages/adapter-next/tests/adapter.test.tspackages/adapter-sveltekit/src/index.tspackages/auth-clerk/src/contracts.tspackages/auth-clerk/src/index.tspackages/auth-clerk/tests/package.test.tspackages/auth-clerk/tests/package.type.test.tspackages/auth/tests/package.test.tspackages/cli/src/project/scaffold/framework-renderers.tspackages/cli/tests/cli.test.tspackages/config/src/defaults.tspackages/config/tests/config.test.tspackages/flux-react/tests/package.test.tspackages/forms/tests/contracts.test.tsscripts/publish-with-resolved-catalogs.mjsscripts/publish-with-resolved-catalogs.test.mjs
✅ Files skipped from review due to trivial changes (2)
- packages/auth-clerk/tests/package.type.test.ts
- packages/auth/tests/package.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/docs/docs/auth/clerk.md
- scripts/publish-with-resolved-catalogs.mjs
- packages/adapter-sveltekit/src/index.ts
- package.json
- packages/cli/src/project/scaffold/framework-renderers.ts
- packages/auth-clerk/src/index.ts
- packages/config/src/defaults.ts
- packages/config/tests/config.test.ts
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation