move auth helpers to auth package#27
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 (9)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR extracts framework auth helpers into a new ChangesAuth package extraction & framework integration
Adapter package cleanup
Application migrations & route protection wiring
Tests, helpers & docs
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (Login Page)
participant Form as useForm
participant AuthClient as useAuth
participant API as /api/login
participant AuthServer as /api/auth/user
Browser->>Form: submit credentials
Form->>API: POST /api/login
API-->>Browser: { ok: true, redirectTo }
Browser->>AuthClient: refreshUser()
AuthClient->>AuthServer: GET current-auth
AuthServer-->>AuthClient: HoloAuthUser
AuthClient->>Browser: update user state
Browser->>Browser: navigate to redirectTo
sequenceDiagram
participant User as Browser
participant App as App Server
participant Middleware as guestOnly/authOnly
participant AuthServer as auth()
participant Login as /login
User->>App: GET /admin/posts
App->>Middleware: protectRoutes(...) runs
Middleware->>AuthServer: auth()
AuthServer-->>Middleware: { authenticated: false }
Middleware-->>App: redirect 303 -> /login
App->>Login: GET /login
Login-->>User: login form
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/blog-nuxt/app/pages/login.vue (1)
12-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
refreshUserrejection will silently swallow thenavigateTocall.If
refreshUser()throws (e.g., a transient network error immediately after a successful server-side login), thenavigateToon line 14 is never reached. The user is authenticated server-side but remains stuck on the login page. Prior to this PR,navigateTowas called directly without this intermediate step.🐛 Proposed fix — navigate regardless of refresh outcome
if (submission?.ok === true && typeof submission.data?.redirectTo === 'string') { - await refreshUser() + try { await refreshUser() } catch { /* best-effort; auth state will sync on next page load */ } await navigateTo(submission.data.redirectTo, { external: true, }) }🤖 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/app/pages/login.vue` around lines 12 - 17, The code currently awaits refreshUser() before calling navigateTo(), so if refreshUser() rejects the navigateTo(submission.data.redirectTo) is never executed; update the submit-success path in the login page to ensure navigateTo is invoked regardless of refreshUser outcome by moving navigateTo into a finally block or by catching errors from refreshUser (call refreshUser() in a try/catch and log/ignore errors) and then calling navigateTo(submission.data.redirectTo, { external: true }) unconditionally; reference refreshUser and navigateTo in the login.vue success handling to locate and change the flow.
🧹 Nitpick comments (2)
packages/adapter-nuxt/tests/package.test.ts (1)
29-32: ⚡ Quick winReduce brittleness in dynamic-import assertions.
These
toContain(...)checks are tied to exact formatting (as string, quote style). Prefer pattern matching so refactors don’t break boundary tests without behavior changes.Proposed update
- expect(moduleEntry).toContain("import('@holo-js/storage')") - expect(moduleEntry).toContain("import('@holo-js/storage-s3' as string)") + expect(moduleEntry).toMatch(/import\(['"]@holo-js\/storage['"]\)/) + expect(moduleEntry).toMatch(/import\(['"]@holo-js\/storage-s3['"](?:\s+as\s+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 `@packages/adapter-nuxt/tests/package.test.ts` around lines 29 - 32, The assertions in package.test.ts are brittle because they depend on exact formatting like quote style and the literal "as string"; update the assertions to use pattern matching (regex) against moduleEntry and runtimeEntry instead of exact substrings: replace the toContain checks for import('@holo-js/storage') and import('@holo-js/storage-s3' as string) with regex-based expectations (e.g., expecting an import of `@holo-js/storage` or `@holo-js/storage-s3` regardless of quotes or whitespace), and keep the negative check for "@holo-js/forms" but use a regex/contains check that is tolerant to formatting changes; update references to moduleEntry and runtimeEntry accordingly (use expect(...).toMatch or expect.stringMatching).apps/docs/docs/forms/server-validation.md (1)
247-387: 💤 Low valueConsider adding a SvelteKit
useForm+useAuthtab for parity.The Next.js and Nuxt tabs in this code-group now both demonstrate
useFormcombined withuseAuth(and the post-submitrefreshUser()call). The SvelteKit tab shows a form actions pattern instead, which is entirely server-side and doesn't useuseAuth. A reader following the SvelteKit path won't find the equivalent client-sideuseAuthfrom@holo-js/auth/sveltekit/clientusage here — they'd have to look elsewhere.Adding a second SvelteKit tab (e.g.,
[SvelteKit — useForm client]) that mirrors the Next.js/Nuxt pattern withuseAuthfrom@holo-js/auth/sveltekit/clientwould make the three frameworks comparable in this section.🤖 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/forms/server-validation.md` around lines 247 - 387, Add a second SvelteKit tab demonstrating the client-side useForm + useAuth pattern to match Next.js/Nuxt: create a "[SvelteKit — useForm client]" example that imports useAuth from '@holo-js/auth/sveltekit/client' and useForm from '@holo-js/adapter-sveltekit/client', initializes the form with loginForm and csrf/initialValues, implements submitter which posts formData and calls refreshUser() (or auth.refreshUser()) on success, and binds form.values.*, uses form.fields.email.onInput/onBlur etc and form.submit() in the template so readers can see the same client-side flow; ensure the tab mirrors the symbols used in the other examples (useAuth, useForm, loginForm, submitter, refreshUser, form.fields.*, form.values.*, form.submit).
🤖 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/blog-next/tests/run.mjs`:
- Around line 157-183: The waitForRedirect loop can hang if a single fetch never
resolves; to fix, add an AbortController for each fetch inside waitForRedirect
and pass its signal to fetch, scheduling a per-request timeout (e.g., timeout =
Math.max(100, Math.min(remainingMs, 5000))) using setTimeout to call
controller.abort(); compute remainingMs = timeoutMs - (Date.now() - startedAt)
before each request, clear the timeout after fetch completes, and handle abort
errors in the existing catch so the overall timeout still stops the loop —
update the fetch call and surrounding try/catch in waitForRedirect accordingly.
In `@apps/blog-sveltekit/tests/run.mjs`:
- Around line 156-182: The polling helper waitForRedirect can hang because an
in-flight fetch is not bounded by timeoutMs; modify waitForRedirect to create an
AbortController for each fetch call, pass controller.signal into fetch, and set
a per-request timer (e.g., Math.min(remainingTime, 5000) or a fixed 5s) that
calls controller.abort() when it expires; clear that timer in a finally block so
aborted fetches throw and are caught by the existing catch block (preserve
lastError handling) and ensure you compute remainingTime from startedAt to avoid
overrunning the overall timeoutMs.
In `@packages/auth/src/next/client.ts`:
- Around line 62-73: The hook treats any passed options object (e.g. {}) as
truthy causing refreshOnMount to be true and bypassing the provider; update
useAuth to treat only an absent options argument as "no options": replace
Boolean(options) with options === undefined in the refreshOnMount expression and
change the provider short-circuit to check options === undefined (i.e. if
(options === undefined && context) return context) so useAuth({}) behaves the
same as useAuth().
In `@packages/auth/src/next/server.ts`:
- Around line 75-79: The isSameUrl function currently compares only pathname,
search and hash and therefore treats URLs with different origins as equal;
update isSameUrl to also compare left.origin === right.origin so the function
returns false when origins differ. Locate the isSameUrl function and add origin
to the equality check (alongside pathname, search and hash) so absolute
redirectTo values on other origins are not treated as self-redirects.
In `@packages/auth/src/nuxt/server.ts`:
- Around line 68-70: The isSamePath guard in isSamePath(path: string,
redirectTo: string) compares normalizePathname(path) to
normalizePathname(redirectTo) but fails when redirectTo contains a query or hash
(e.g., "/login?returnUrl=..."), causing redirect loops; update isSamePath to
first strip query string and fragment from redirectTo (and optionally path)
before normalizing and comparing — e.g., parse redirectTo to extract only the
pathname (using URL or string split on '?' / '#') and then call
normalizePathname on that value so "/login" and "/login?returnUrl=..." are
treated as the same path.
---
Outside diff comments:
In `@apps/blog-nuxt/app/pages/login.vue`:
- Around line 12-17: The code currently awaits refreshUser() before calling
navigateTo(), so if refreshUser() rejects the
navigateTo(submission.data.redirectTo) is never executed; update the
submit-success path in the login page to ensure navigateTo is invoked regardless
of refreshUser outcome by moving navigateTo into a finally block or by catching
errors from refreshUser (call refreshUser() in a try/catch and log/ignore
errors) and then calling navigateTo(submission.data.redirectTo, { external: true
}) unconditionally; reference refreshUser and navigateTo in the login.vue
success handling to locate and change the flow.
---
Nitpick comments:
In `@apps/docs/docs/forms/server-validation.md`:
- Around line 247-387: Add a second SvelteKit tab demonstrating the client-side
useForm + useAuth pattern to match Next.js/Nuxt: create a "[SvelteKit — useForm
client]" example that imports useAuth from '@holo-js/auth/sveltekit/client' and
useForm from '@holo-js/adapter-sveltekit/client', initializes the form with
loginForm and csrf/initialValues, implements submitter which posts formData and
calls refreshUser() (or auth.refreshUser()) on success, and binds form.values.*,
uses form.fields.email.onInput/onBlur etc and form.submit() in the template so
readers can see the same client-side flow; ensure the tab mirrors the symbols
used in the other examples (useAuth, useForm, loginForm, submitter, refreshUser,
form.fields.*, form.values.*, form.submit).
In `@packages/adapter-nuxt/tests/package.test.ts`:
- Around line 29-32: The assertions in package.test.ts are brittle because they
depend on exact formatting like quote style and the literal "as string"; update
the assertions to use pattern matching (regex) against moduleEntry and
runtimeEntry instead of exact substrings: replace the toContain checks for
import('@holo-js/storage') and import('@holo-js/storage-s3' as string) with
regex-based expectations (e.g., expecting an import of `@holo-js/storage` or
`@holo-js/storage-s3` regardless of quotes or whitespace), and keep the negative
check for "@holo-js/forms" but use a regex/contains check that is tolerant to
formatting changes; update references to moduleEntry and runtimeEntry
accordingly (use expect(...).toMatch or expect.stringMatching).
🪄 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: b0dace85-f9a1-407d-bb5a-b86191a93130
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (70)
apps/blog-next/app/auth-nav.tsxapps/blog-next/app/layout.tsxapps/blog-next/app/login/page.tsxapps/blog-next/app/verify-email/page.tsxapps/blog-next/proxy.tsapps/blog-next/tests/run.mjsapps/blog-nuxt/app/app.vueapps/blog-nuxt/app/middleware/auth-only.global.tsapps/blog-nuxt/app/middleware/guest-only.global.tsapps/blog-nuxt/app/pages/login.vueapps/blog-nuxt/app/pages/register.vueapps/blog-nuxt/app/pages/verify-email.vueapps/blog-nuxt/tests/run.mjsapps/blog-sveltekit/src/hooks.server.tsapps/blog-sveltekit/src/routes/+layout.server.tsapps/blog-sveltekit/src/routes/+layout.svelteapps/blog-sveltekit/src/routes/login/+page.svelteapps/blog-sveltekit/src/routes/register/+page.svelteapps/blog-sveltekit/src/routes/verify-email/+page.svelteapps/blog-sveltekit/tests/run.mjsapps/docs/docs/auth/current-auth-client.mdapps/docs/docs/auth/index.mdapps/docs/docs/forms/framework-integration.mdapps/docs/docs/forms/server-validation.mdpackages/adapter-next/package.jsonpackages/adapter-next/src/client.tspackages/adapter-next/src/request-context.tspackages/adapter-next/src/server.tspackages/adapter-next/tests/adapter.type.test.tspackages/adapter-next/tests/client.test.tspackages/adapter-next/tests/package.test.tspackages/adapter-next/tests/server.test.tspackages/adapter-next/tsconfig.jsonpackages/adapter-next/tsup.config.tspackages/adapter-next/vitest.config.tspackages/adapter-nuxt/package.jsonpackages/adapter-nuxt/src/module.tspackages/adapter-nuxt/src/runtime/composables/auth.d.tspackages/adapter-nuxt/src/runtime/server/protection.d.tspackages/adapter-nuxt/src/runtime/shims.d.tspackages/adapter-nuxt/tests/client.test.tspackages/adapter-nuxt/tests/module.test.tspackages/adapter-nuxt/tests/package.test.tspackages/adapter-nuxt/tests/protection.test.tspackages/adapter-nuxt/tests/setup.test.tspackages/adapter-nuxt/tsconfig.jsonpackages/adapter-nuxt/vitest.config.tspackages/adapter-sveltekit/package.jsonpackages/adapter-sveltekit/src/client.tspackages/adapter-sveltekit/tests/adapter.type.test.tspackages/adapter-sveltekit/tests/client.test.tspackages/adapter-sveltekit/tests/package.test.tspackages/adapter-sveltekit/tests/server.test.tspackages/adapter-sveltekit/tsconfig.jsonpackages/adapter-sveltekit/tsup.config.tspackages/adapter-sveltekit/vitest.config.tspackages/auth/package.jsonpackages/auth/src/next/client.tspackages/auth/src/next/request-context.tspackages/auth/src/next/server.tspackages/auth/src/nuxt-shim.d.tspackages/auth/src/nuxt.tspackages/auth/src/nuxt/server.tspackages/auth/src/sveltekit/client.tspackages/auth/src/sveltekit/server.tspackages/auth/tests/contracts.type.test.tspackages/auth/tests/docs-smoke.test.tspackages/auth/tests/package.test.tspackages/auth/tsup.config.tstests/example-app-auth-flow.mjs
💤 Files with no reviewable changes (21)
- packages/adapter-nuxt/src/runtime/server/protection.d.ts
- packages/adapter-next/tests/client.test.ts
- packages/adapter-sveltekit/tsup.config.ts
- packages/adapter-next/package.json
- packages/adapter-sveltekit/package.json
- packages/adapter-nuxt/src/runtime/composables/auth.d.ts
- packages/adapter-nuxt/tsconfig.json
- packages/adapter-nuxt/tests/client.test.ts
- packages/adapter-nuxt/package.json
- packages/adapter-next/tests/server.test.ts
- packages/adapter-nuxt/vitest.config.ts
- packages/adapter-sveltekit/tests/server.test.ts
- packages/adapter-next/tsup.config.ts
- packages/adapter-sveltekit/tsconfig.json
- packages/adapter-next/src/server.ts
- packages/adapter-next/vitest.config.ts
- packages/adapter-sveltekit/vitest.config.ts
- packages/adapter-next/tsconfig.json
- packages/adapter-nuxt/tests/protection.test.ts
- packages/adapter-nuxt/src/module.ts
- packages/adapter-sveltekit/src/client.ts
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
This was referenced May 7, 2026
Merged
Merged
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
Improvements
Documentation