fix auth client helpers#23
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds first-class authentication across Next, Nuxt, and SvelteKit adapters: new client hooks/providers (useAuth / AuthProvider), server auth facades (auth()), HoloAuthUser type alias, example app UI components, package exports/configs, docs updates, and extensive tests and test harness improvements. ChangesAuthentication integration (single DAG across adapters, client+server+apps+tests/docs)
Sequence DiagramsequenceDiagram
participant Client as Client App
participant Hook as useAuth / AuthProvider
participant Endpoint as /api/auth/user or /api/logout
participant Runtime as Auth Runtime (server)
Client->>Hook: initialize / render
Hook->>Runtime: request current user (server-side or client fetch)
Runtime-->>Hook: current user (HoloAuthUser or null)
Hook->>Client: provide authenticated state & user
alt Authenticated
Client->>Endpoint: POST /api/logout
Endpoint->>Runtime: perform logout (hydrate session if needed)
Runtime-->>Endpoint: success
Endpoint-->>Client: 204
Client->>Hook: refreshUser()
Hook->>Runtime: refetch current user
Runtime-->>Hook: null (guest)
Hook->>Client: update UI to unauthenticated
else Unauthenticated
Client->>Client: show Login/Register links
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (16)
tests/example-app-auth-flow.mjs (1)
136-136: 💤 Low valueConsider referencing
DEFAULT_SESSION_COOKIE_NAMEinstead of the inline literal.
'holo_session'is duplicated here and inpackages/session/src/runtime.ts. The single source of truth isDEFAULT_SESSION_COOKIE_NAMEinpackages/config/src/defaults.ts. If the default ever changes, this assertion silently drifts.🤖 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 `@tests/example-app-auth-flow.mjs` at line 136, Replace the hardcoded session cookie name literal used for sessionCookieName with the shared constant DEFAULT_SESSION_COOKIE_NAME so the test references the single source of truth; update the assignment where sessionCookieName is defined to import/use DEFAULT_SESSION_COOKIE_NAME (the same constant used by the session runtime) and remove the duplicated string literal to prevent drift if the default changes.packages/adapter-next/tests/package.test.ts (1)
6-6: 💤 Low valueConsider updating the test name to reflect its expanded scope.
Same as the corresponding SvelteKit test:
'keeps the forms client surface as an optional peer'no longer describes the full scope — the test now also validates the@holo-js/auth/clientimport surface, the@holo-js/authdependency, and thereactpeer dependency.📝 Suggested rename
- it('keeps the forms client surface as an optional peer', async () => { + it('keeps forms as an optional peer, auth as a direct dep, and react as a peer', async () => {🤖 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-next/tests/package.test.ts` at line 6, Rename the test whose title is 'keeps the forms client surface as an optional peer' to reflect its broader assertions (it now also validates the `@holo-js/auth/client` import surface, the `@holo-js/auth` dependency, and the react peer dependency); update the string in the it(...) call in package.test.ts to a more descriptive name such as 'keeps forms client, `@holo-js/auth` client surface, `@holo-js/auth` dependency, and react as optional/peer dependencies' so the test title matches the expanded scope.packages/adapter-sveltekit/tests/package.test.ts (1)
6-6: 💤 Low valueConsider updating the test name to reflect its expanded scope.
The test name
'keeps the forms client surface as an optional peer'no longer fully describes what the test verifies — it now also asserts the presence of@holo-js/auth/clientin the client entry and@holo-js/authin the dependencies.📝 Suggested rename
- it('keeps the forms client surface as an optional peer', async () => { + it('keeps forms as an optional peer and auth as a direct dependency', async () => {🤖 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-sveltekit/tests/package.test.ts` at line 6, Update the test title in the it(...) call currently named 'keeps the forms client surface as an optional peer' to reflect its expanded assertions — rename it to something like 'keeps the forms client surface as an optional peer and includes `@holo-js/auth` in client entry and dependencies' so the test (in packages/adapter-sveltekit/tests/package.test.ts) clearly indicates it now also checks for `@holo-js/auth/client` in the client entry and `@holo-js/auth` in the dependencies.packages/adapter-nuxt/tests/module.test.ts (1)
338-343: ⚡ Quick winAdd an explicit assertion for the new
useAuthimport.The
toHaveLength(7)guard prevents accidental count drift, but thearrayContainingcheck doesn't verify thatuseAuthis the actual 7th entry. If the import were misconfigured or replaced, the test would still pass as long as the count is 7.📝 Suggested addition
expect(addImports.mock.calls[0]?.[0]).toEqual(expect.arrayContaining([ expect.objectContaining({ name: 'holo', as: 'holo', from: '@holo-js/adapter-nuxt/runtime' }), expect.objectContaining({ name: 'useStorage', as: 'useStorage', from: '@holo-js/adapter-nuxt/storage' }), expect.objectContaining({ name: 'Storage', as: 'Storage', from: '@holo-js/adapter-nuxt/storage' }), + expect.objectContaining({ name: 'useAuth', as: 'useAuth', from: '@holo-js/adapter-nuxt/auth' }), ]))🤖 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/module.test.ts` around lines 338 - 343, Add an explicit assertion that the 7th import is the new useAuth entry: reference addImports.mock.calls[0]?.[0][6] and assert it equals an object containing { name: 'useAuth', as: 'useAuth', from: '@holo-js/adapter-nuxt/auth' } so the test ensures useAuth is actually present in the expected position.packages/adapter-sveltekit/src/server.ts (1)
13-15: 💤 Low value
toClientAuthUsershallow-copies but does not deep-sanitize nested values
{ ...user }strips the prototype chain and detaches reactive/proxy wrappers, which is the primary value here. However, any nested non-primitive values stored under the[key: string]: unknownindex signature (e.g.Date, class instances, circular refs) are carried through by reference. In a SvelteKit serverloadfunction these will cause a serialization error at runtime if a consuming app puts non-serializable values in custom auth-user fields.Consider documenting this constraint, or adding a note that callers must ensure all user fields are serializable.
🤖 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-sveltekit/src/server.ts` around lines 13 - 15, toClientAuthUser currently returns a shallow copy ({ ...user }) which still preserves nested non-primitive references (Dates, class instances, circular refs) and can cause serialization errors in SvelteKit load functions; change the implementation of toClientAuthUser to return a serialization-safe deep copy (e.g. serialize/deserialize via JSON or otherwise strip/convert non-primitive fields) so only plain serializable data is returned, and document that callers must avoid non-serializable custom fields if you choose to only document the constraint; reference: function name toClientAuthUser.packages/adapter-sveltekit/src/client.ts (1)
33-74: ⚡ Quick winClarify SvelteKit pattern:
useAuth()per call creates independent state; document the "call once in layout" requirement.The per-call
useAuth()behavior is intentional per the documented pattern—the example in both the guide and the blog app callsuseAuth()once in+layout.svelteand passes the result down. However, the documentation doesn't explicitly state this as a requirement. A nested component that importsuseAuth()directly will silently create a separateAuthClientStateinstance and desync onrefreshUser()calls, which is a real footgun given the SvelteKit adapter lacks anAuthProviderwrapper like the Next.js adapter or an exportedsetContext/getContexthelper.Either document that
useAuth()must be called exactly once at the top level and shared via template context, or provide a context helper (e.g.setAuthContext/getAuthContext) so multiple components can safely share oneAuthClientStateinstance without silent breakage.🤖 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-sveltekit/src/client.ts` around lines 33 - 74, The current useAuth() creates a new AuthClientState per call which can silently desync; add a Svelte context helper pair (e.g., setAuthContext and getAuthContext) and update useAuth to check getAuthContext first and return the existing AuthClientState if present, otherwise create a new AuthClientState, call setAuthContext with it, and return it; reference the AuthClientState class, useAuth function, and new setAuthContext/getAuthContext helpers so consumers can call setAuthContext in +layout.svelte once (or rely on the automatic behavior) to ensure a single shared instance across components.packages/adapter-sveltekit/tests/adapter.type.test.ts (1)
26-39: 💤 Low valuePreserve original error context in
expectCommandToPass.When the spawned command produces no stdout/stderr (e.g., the binary itself is missing or fails before writing to streams), the resulting
Errorhas an empty message and the original error chain is dropped, which makes test failures hard to diagnose. Consider preserving the source error via thecauseoption and falling back to the original message:♻️ Proposed refactor
function expectCommandToPass(command: string, args: readonly string[], options: ExecFileSyncOptionsWithBufferEncoding): void { try { execFileSync(command, args, options) } catch (error) { const failure = error as { + readonly message?: string readonly stderr?: Buffer readonly stdout?: Buffer } - throw new Error([ - failure.stdout?.toString(), - failure.stderr?.toString(), - ].filter(Boolean).join('\n')) + const output = [ + failure.stdout?.toString(), + failure.stderr?.toString(), + ].filter(Boolean).join('\n') + throw new Error(output || failure.message || 'Command failed', { cause: error }) } }🤖 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-sveltekit/tests/adapter.type.test.ts` around lines 26 - 39, The test helper expectCommandToPass currently swallows the original thrown error and only builds a message from stdout/stderr, which can produce empty messages and lose the error chain; update the catch block in expectCommandToPass to include the original error as the cause when throwing (use the Error constructor's cause option) and fall back to the original error.message if stdout/stderr are empty so that the thrown Error preserves the original execFileSync error and its message/stack for easier debugging.packages/adapter-next/tests/client.test.ts (3)
137-178: 💤 Low value
Promise.resolve()may not flush all microtasks queued byrefreshUser.
useEffectin this mock invokes the effect synchronously, which callsvoid refreshUser().refreshUseris async and awaitsrefreshCurrentUser(...). A singleawait Promise.resolve()may not be enough to settle the chain (async fn → await mockedAsync → setCurrentUser). It happens to work becauserefreshCurrentUseris mocked as a single-await async function, but if more awaits are added in the implementation, this test will silently start asserting against the pre-refresh state.A safer pattern is to flush microtasks with a small helper or
await new Promise(setImmediate)/await new Promise(r => setTimeout(r, 0)), or to await the returned promise fromrefreshUserdirectly.🤖 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-next/tests/client.test.ts` around lines 137 - 178, The test can race because awaiting only Promise.resolve() may not flush all microtasks started by the async refreshUser called inside the mocked useEffect; update the test to wait for the actual async work by either awaiting the refreshUser promise directly (capture and await the promise returned by the mocked refreshUser) or by flushing microtasks (e.g., await new Promise(resolve => setImmediate(resolve)) ) after invoking useAuth() so that the chain refreshUser → refreshCurrentUser → state update completes before asserting hookState equals refreshedUser; refer to the mocked refreshUser and the useAuth() call and ensure hookState is asserted only after the awaited flush.
9-37: 💤 Low value
createReactMockis a clean abstraction; a couple of small notes.
- The default
useContextreturns the latestcontext.value(mutated byProvider), which is enough for these tests but does not mirror React's render-time context resolution. As the auth surface grows (e.g., nested providers, conditional rendering), tests may need a stricter fake.createElementinvokes function components synchronously and mergeschildrenas a single value when there's one child but as an array when there are multiple. Real React always passes children as a single value (or array). This inconsistency is harmless here, but if a future component doesChildren.count(children)or similar, the mock won't match.🤖 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-next/tests/client.test.ts` around lines 9 - 37, createReactMock's useContext and createElement differ from real React: useContext reads a mutated context.value which won't model render-time snapshots, and createElement sometimes returns children as raw value vs array inconsistently. Update createReactMock so MockReactContext stores a currentRenderValue (or an immutable snapshot per render) and Provider sets that snapshot to emulate render-time resolution for useContext; and ensure createElement always passes children as a single prop (wrap in an array only when appropriate by using a consistent single "children" value like children.length === 1 ? children[0] : children) but normalize to always pass the same shape (e.g., always an array or always a single value) to match real React semantics used by components; adjust MockReactContext/Provider and the createElement implementation accordingly to keep behavior consistent across tests involving nested providers or Children utilities (referencing createReactMock, MockReactContext, Provider, useContext, and createElement).
96-135: ⚡ Quick winVerify shared-provider test actually exercises the shared-context branch.
This test relies on
useAuth()(no options) returning the provider's value viauseContext. Given theuseAuthimplementation issue flagged inclient.ts(conditional hook invocation), this test passes here because theuseState/useRef/useCallback/useEffectmocks don't enforce hook order. In real React, callinguseAuth()afterAuthProviderwas rendered separately wouldn't actually share state —AuthProvider'suseAuthStatekeeps itscurrentUserin its own component, not in the context. The Provider only places the initially-computedauthobject (with asetCurrentUserbound to that component) into context. So this test arguably tests the mock more than the production behavior.Consider augmenting with an integration-style test using
@testing-library/reactonceuseAuth's hook-order issue is fixed, to confirm shared state semantics under real React.🤖 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-next/tests/client.test.ts` around lines 96 - 135, The failing test masks a real bug in client.ts where useAuth conditionally invokes hooks / computes and places only an initially-computed auth object into context instead of sharing the provider's live state; update the implementation so hooks are called unconditionally and the Provider stores and exposes the actual useAuthState values (currentUser and setCurrentUser) in context rather than a snapshot—specifically, refactor AuthProvider to call useAuthState inside the provider component and pass { authenticated, user: currentUser, setCurrentUser, ... } (or the same auth object returned by useAuthState) into the context, and change useAuth to always call React hooks in the same order and return the context value; after that, replace or augment the unit test with an integration test using `@testing-library/react` to assert that useAuth() consumers share and update the provider state.packages/adapter-nuxt/src/runtime/shims.d.ts (2)
57-60: ⚡ Quick winDefault
useFetchgeneric toCurrentAuthResponsecouples a general-purpose shim to the auth surface.Defaulting
TValuetoCurrentAuthResponsemakes anyuseFetch(url)call without an explicit generic silently typed as the auth response. That's misleading for non-auth callers and creates a leaky dependency from the Nuxt runtime shim to@holo-js/auth. If only the auth composable needs this shape, prefer either dropping the default (let callers specify) or introducing a dedicated auth-aware helper that re-exportsuseFetchwith the bound type.♻️ Proposed refactor
- export function useFetch<TValue = CurrentAuthResponse>( + export function useFetch<TValue = unknown>( request: string, options?: { readonly key?: string }, ): Promise<HoloUseFetchResult<TValue>>🤖 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/src/runtime/shims.d.ts` around lines 57 - 60, The shim's useFetch signature currently defaults TValue to CurrentAuthResponse which leaks an auth type into all callers; change the declaration of useFetch so it does not default to CurrentAuthResponse (make it a plain generic like useFetch<TValue> or default to unknown/any) and remove the direct coupling to CurrentAuthResponse from the shim; then add a dedicated auth helper (e.g., export function useAuthFetch or an auth-specific re-export) that binds/useFetch<CurrentAuthResponse> for auth consumers so only that helper relies on CurrentAuthResponse; update references to useFetch in auth-related code to use the new auth helper.
38-64: ⚖️ Poor tradeoffNuxt shims are correctly sized for current usage; consider documenting which fields are actually consumed.
The minimal surface here is intentional—
HoloUseFetchResultprovides onlydataandrefresh, anduseFetch'soptionsaccepts onlykey. Verification showscomposables/auth.ts(the only consumer of these shims) uses exactly these fields and options: it destructuresdataandrefreshfrom the result, passes only{ key }in options, and never accessespending,status,error,execute, or otherAsyncDataproperties. Similarly,useStateandcomputedare used with onlykey/getterandvalueaccess. The shims currently align with actual consumption. If future code in the adapter needs additionalAsyncDatafields oruseFetchoptions, the shims should be expanded to match.🤖 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/src/runtime/shims.d.ts` around lines 38 - 64, Add a short inline comment/JSDoc in this shims file documenting that the types are intentionally minimal and list the exact fields consumed by the adapter (e.g., HoloUseFetchResult exposes only data and refresh, useFetch accepts only options.key, useState is used with key/init and HoloRef.value, and computed is used for getter/value), and reference the consumer composable (composables/auth.ts) so future maintainers know why HoloUseFetchResult, useFetch, useState, computed, HoloRef, and HoloComputedRef were kept minimal and when to expand them.apps/blog-sveltekit/src/routes/+layout.svelte (1)
12-20: 💤 Low valueSilent logout failure leaves the user without feedback.
If
/api/logoutreturns a non-OK response,logout()silently returns without any UI signal, and the user stays in the authenticated state with no indication of what happened. For an example/blog app this is acceptable, but in real flows you'll typically want at least a console warning or a toast. Also worth considering: catch network/throw errors fromfetchitself, which currently propagate as unhandled rejections.♻️ Optional improvement
async function logout() { - const response = await fetch('/api/logout', { method: 'POST' }) - if (!response.ok) { - return - } - - await auth.refreshUser() - await invalidateAll() + try { + const response = await fetch('/api/logout', { method: 'POST' }) + if (!response.ok) { + console.warn('Logout failed:', response.status) + return + } + await Promise.all([auth.refreshUser(), invalidateAll()]) + } catch (error) { + console.warn('Logout request failed:', error) + } }🤖 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-sveltekit/src/routes/`+layout.svelte around lines 12 - 20, The logout() function currently returns silently on non-OK responses and doesn't handle fetch errors; wrap the fetch call in a try/catch, check response.ok and on failure emit feedback (e.g., console.warn or trigger your toast/notification) and do not call auth.refreshUser() or invalidateAll() unless the logout succeeded; ensure network/fetch exceptions are caught and similarly reported to the user/developer. Target the logout() function and its flow around auth.refreshUser() and invalidateAll() when implementing this change.packages/adapter-next/src/client.ts (1)
27-29: ⚖️ Poor tradeoff
UseAuthOptionsmay unintentionally surfaceAuthClientRequestOptionskeys to consumers.Spreading
AuthClientRequestOptionsinto the publicUseAuthOptionsexposes whatever shape that type has today across the entire React surface (includingAuthProviderProps). If@holo-js/auth/clientlater adds fields, they implicitly become part of the adapter's API and will land inAuthProviderprops as well. Consider either nesting the request options under a single property (e.g.,request?: AuthClientRequestOptions) or explicitly picking the keys you intend to expose.🤖 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-next/src/client.ts` around lines 27 - 29, The public UseAuthOptions type currently spreads AuthClientRequestOptions which unintentionally leaks all of that type into the adapter API (and into AuthProviderProps); change UseAuthOptions to avoid spreading the external type directly—either wrap it under a single property (e.g., request?: AuthClientRequestOptions) or explicitly Pick the specific keys you want to expose—update the type declaration for UseAuthOptions and any places that construct or consume it (e.g., AuthProviderProps and any call sites) to use the new nested or picked shape so future additions to AuthClientRequestOptions won’t leak into your public props.packages/adapter-sveltekit/tests/client.test.ts (1)
4-16: 💤 Low valueSubscriber cleanup mock semantics may differ from real Svelte behavior.
The mock now invokes the
startcleanup synchronously on the first read. In real Svelte, the cleanup returned fromcreateSubscriber'sstartcallback runs when there are no remaining subscribers (effect teardown), not immediately upon first subscription. This is fine for the current test goals (verifying thatuseAuth.refreshUser()works before any reactive subscriber takes hold), but be aware it may mask code paths that rely on the cleanup being deferred.🤖 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-sveltekit/tests/client.test.ts` around lines 4 - 16, The mock for createSubscriber incorrectly runs the start cleanup synchronously on first invocation; update the mock in tests so createSubscriber(start) stores the cleanup returned by start and only invokes that cleanup when the returned unsubscribe function is called (i.e., on teardown), not immediately on first read—preserve the initialized/subscriber tracking logic and ensure this change still allows useAuth.refreshUser() to be exercised before any subscriber is active.packages/adapter-nuxt/tests/setup.test.ts (1)
424-429: ⚡ Quick winAssert
useAuthexplicitly instead of only checking list length.The
toHaveLength(7)check can still pass ifuseAuthdisappears and another import is added. Add an explicit matcher foruseAuthto lock the contract.Proposed test hardening
expect(addImports.mock.calls[0]?.[0]).toHaveLength(7) expect(addImports.mock.calls[0]?.[0]).toEqual(expect.arrayContaining([ expect.objectContaining({ name: 'holo', as: 'holo', from: '@holo-js/adapter-nuxt/runtime' }), + expect.objectContaining({ name: 'useAuth', as: 'useAuth', from: '@holo-js/adapter-nuxt/auth' }), expect.objectContaining({ name: 'useStorage', as: 'useStorage', from: '@holo-js/adapter-nuxt/storage' }), expect.objectContaining({ name: 'Storage', as: 'Storage', from: '@holo-js/adapter-nuxt/storage' }), ]))🤖 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/setup.test.ts` around lines 424 - 429, The test currently asserts addImports.mock.calls[0]?.[0] has length 7 but doesn't explicitly verify the presence of the useAuth import; update the assertions to include an explicit matcher for useAuth (e.g., expect.objectContaining({ name: 'useAuth', as: 'useAuth', from: '@holo-js/adapter-nuxt/runtime' })) alongside the existing objectContaining checks for holo/useStorage/Storage so the contract cannot be accidentally broken by swapping imports.
🤖 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/app/auth-nav.tsx`:
- Around line 24-31: The logout function is async and passed directly to React
event handlers (logout and its onClick usage), which can produce unhandled
promise rejections if fetch or auth.refreshUser() fails; wrap the call so errors
are caught — either change the logout implementation to catch and handle errors
internally (try/catch around fetch and auth.refreshUser) or replace
onClick={logout} with a safe wrapper that invokes logout() and handles
rejections (e.g., () => void logout().catch(...)) and log or surface the error
via the existing logger/UI; reference the logout function and auth.refreshUser
when making the change.
In `@packages/adapter-next/src/client.ts`:
- Around line 75-82: useAuth currently conditionally returns the context or the
result of useAuthState which changes the number of hooks called across renders;
always call the same hooks by invoking useAuthState(options) unconditionally
alongside useContext(AuthContext), then choose which value to return (e.g.,
return context when !options && context else return the useAuthState result). If
running useAuthState when a provider exists causes unwanted side-effects, modify
useAuthState to accept a flag (or inspect options) and gate its internal
useEffect/useCallback initialization logic rather than avoiding the hook call.
Ensure references to useAuth, useAuthState, and AuthContext are updated
accordingly.
---
Nitpick comments:
In `@apps/blog-sveltekit/src/routes/`+layout.svelte:
- Around line 12-20: The logout() function currently returns silently on non-OK
responses and doesn't handle fetch errors; wrap the fetch call in a try/catch,
check response.ok and on failure emit feedback (e.g., console.warn or trigger
your toast/notification) and do not call auth.refreshUser() or invalidateAll()
unless the logout succeeded; ensure network/fetch exceptions are caught and
similarly reported to the user/developer. Target the logout() function and its
flow around auth.refreshUser() and invalidateAll() when implementing this
change.
In `@packages/adapter-next/src/client.ts`:
- Around line 27-29: The public UseAuthOptions type currently spreads
AuthClientRequestOptions which unintentionally leaks all of that type into the
adapter API (and into AuthProviderProps); change UseAuthOptions to avoid
spreading the external type directly—either wrap it under a single property
(e.g., request?: AuthClientRequestOptions) or explicitly Pick the specific keys
you want to expose—update the type declaration for UseAuthOptions and any places
that construct or consume it (e.g., AuthProviderProps and any call sites) to use
the new nested or picked shape so future additions to AuthClientRequestOptions
won’t leak into your public props.
In `@packages/adapter-next/tests/client.test.ts`:
- Around line 137-178: The test can race because awaiting only Promise.resolve()
may not flush all microtasks started by the async refreshUser called inside the
mocked useEffect; update the test to wait for the actual async work by either
awaiting the refreshUser promise directly (capture and await the promise
returned by the mocked refreshUser) or by flushing microtasks (e.g., await new
Promise(resolve => setImmediate(resolve)) ) after invoking useAuth() so that the
chain refreshUser → refreshCurrentUser → state update completes before asserting
hookState equals refreshedUser; refer to the mocked refreshUser and the
useAuth() call and ensure hookState is asserted only after the awaited flush.
- Around line 9-37: createReactMock's useContext and createElement differ from
real React: useContext reads a mutated context.value which won't model
render-time snapshots, and createElement sometimes returns children as raw value
vs array inconsistently. Update createReactMock so MockReactContext stores a
currentRenderValue (or an immutable snapshot per render) and Provider sets that
snapshot to emulate render-time resolution for useContext; and ensure
createElement always passes children as a single prop (wrap in an array only
when appropriate by using a consistent single "children" value like
children.length === 1 ? children[0] : children) but normalize to always pass the
same shape (e.g., always an array or always a single value) to match real React
semantics used by components; adjust MockReactContext/Provider and the
createElement implementation accordingly to keep behavior consistent across
tests involving nested providers or Children utilities (referencing
createReactMock, MockReactContext, Provider, useContext, and createElement).
- Around line 96-135: The failing test masks a real bug in client.ts where
useAuth conditionally invokes hooks / computes and places only an
initially-computed auth object into context instead of sharing the provider's
live state; update the implementation so hooks are called unconditionally and
the Provider stores and exposes the actual useAuthState values (currentUser and
setCurrentUser) in context rather than a snapshot—specifically, refactor
AuthProvider to call useAuthState inside the provider component and pass {
authenticated, user: currentUser, setCurrentUser, ... } (or the same auth object
returned by useAuthState) into the context, and change useAuth to always call
React hooks in the same order and return the context value; after that, replace
or augment the unit test with an integration test using `@testing-library/react`
to assert that useAuth() consumers share and update the provider state.
In `@packages/adapter-next/tests/package.test.ts`:
- Line 6: Rename the test whose title is 'keeps the forms client surface as an
optional peer' to reflect its broader assertions (it now also validates the
`@holo-js/auth/client` import surface, the `@holo-js/auth` dependency, and the react
peer dependency); update the string in the it(...) call in package.test.ts to a
more descriptive name such as 'keeps forms client, `@holo-js/auth` client surface,
`@holo-js/auth` dependency, and react as optional/peer dependencies' so the test
title matches the expanded scope.
In `@packages/adapter-nuxt/src/runtime/shims.d.ts`:
- Around line 57-60: The shim's useFetch signature currently defaults TValue to
CurrentAuthResponse which leaks an auth type into all callers; change the
declaration of useFetch so it does not default to CurrentAuthResponse (make it a
plain generic like useFetch<TValue> or default to unknown/any) and remove the
direct coupling to CurrentAuthResponse from the shim; then add a dedicated auth
helper (e.g., export function useAuthFetch or an auth-specific re-export) that
binds/useFetch<CurrentAuthResponse> for auth consumers so only that helper
relies on CurrentAuthResponse; update references to useFetch in auth-related
code to use the new auth helper.
- Around line 38-64: Add a short inline comment/JSDoc in this shims file
documenting that the types are intentionally minimal and list the exact fields
consumed by the adapter (e.g., HoloUseFetchResult exposes only data and refresh,
useFetch accepts only options.key, useState is used with key/init and
HoloRef.value, and computed is used for getter/value), and reference the
consumer composable (composables/auth.ts) so future maintainers know why
HoloUseFetchResult, useFetch, useState, computed, HoloRef, and HoloComputedRef
were kept minimal and when to expand them.
In `@packages/adapter-nuxt/tests/module.test.ts`:
- Around line 338-343: Add an explicit assertion that the 7th import is the new
useAuth entry: reference addImports.mock.calls[0]?.[0][6] and assert it equals
an object containing { name: 'useAuth', as: 'useAuth', from:
'@holo-js/adapter-nuxt/auth' } so the test ensures useAuth is actually present
in the expected position.
In `@packages/adapter-nuxt/tests/setup.test.ts`:
- Around line 424-429: The test currently asserts addImports.mock.calls[0]?.[0]
has length 7 but doesn't explicitly verify the presence of the useAuth import;
update the assertions to include an explicit matcher for useAuth (e.g.,
expect.objectContaining({ name: 'useAuth', as: 'useAuth', from:
'@holo-js/adapter-nuxt/runtime' })) alongside the existing objectContaining
checks for holo/useStorage/Storage so the contract cannot be accidentally broken
by swapping imports.
In `@packages/adapter-sveltekit/src/client.ts`:
- Around line 33-74: The current useAuth() creates a new AuthClientState per
call which can silently desync; add a Svelte context helper pair (e.g.,
setAuthContext and getAuthContext) and update useAuth to check getAuthContext
first and return the existing AuthClientState if present, otherwise create a new
AuthClientState, call setAuthContext with it, and return it; reference the
AuthClientState class, useAuth function, and new setAuthContext/getAuthContext
helpers so consumers can call setAuthContext in +layout.svelte once (or rely on
the automatic behavior) to ensure a single shared instance across components.
In `@packages/adapter-sveltekit/src/server.ts`:
- Around line 13-15: toClientAuthUser currently returns a shallow copy ({
...user }) which still preserves nested non-primitive references (Dates, class
instances, circular refs) and can cause serialization errors in SvelteKit load
functions; change the implementation of toClientAuthUser to return a
serialization-safe deep copy (e.g. serialize/deserialize via JSON or otherwise
strip/convert non-primitive fields) so only plain serializable data is returned,
and document that callers must avoid non-serializable custom fields if you
choose to only document the constraint; reference: function name
toClientAuthUser.
In `@packages/adapter-sveltekit/tests/adapter.type.test.ts`:
- Around line 26-39: The test helper expectCommandToPass currently swallows the
original thrown error and only builds a message from stdout/stderr, which can
produce empty messages and lose the error chain; update the catch block in
expectCommandToPass to include the original error as the cause when throwing
(use the Error constructor's cause option) and fall back to the original
error.message if stdout/stderr are empty so that the thrown Error preserves the
original execFileSync error and its message/stack for easier debugging.
In `@packages/adapter-sveltekit/tests/client.test.ts`:
- Around line 4-16: The mock for createSubscriber incorrectly runs the start
cleanup synchronously on first invocation; update the mock in tests so
createSubscriber(start) stores the cleanup returned by start and only invokes
that cleanup when the returned unsubscribe function is called (i.e., on
teardown), not immediately on first read—preserve the initialized/subscriber
tracking logic and ensure this change still allows useAuth.refreshUser() to be
exercised before any subscriber is active.
In `@packages/adapter-sveltekit/tests/package.test.ts`:
- Line 6: Update the test title in the it(...) call currently named 'keeps the
forms client surface as an optional peer' to reflect its expanded assertions —
rename it to something like 'keeps the forms client surface as an optional peer
and includes `@holo-js/auth` in client entry and dependencies' so the test (in
packages/adapter-sveltekit/tests/package.test.ts) clearly indicates it now also
checks for `@holo-js/auth/client` in the client entry and `@holo-js/auth` in the
dependencies.
In `@tests/example-app-auth-flow.mjs`:
- Line 136: Replace the hardcoded session cookie name literal used for
sessionCookieName with the shared constant DEFAULT_SESSION_COOKIE_NAME so the
test references the single source of truth; update the assignment where
sessionCookieName is defined to import/use DEFAULT_SESSION_COOKIE_NAME (the same
constant used by the session runtime) and remove the duplicated string literal
to prevent drift if the default changes.
🪄 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: 7e63021b-d768-4bde-b0a2-6aadfb51e9b3
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
apps/blog-next/app/auth-nav.tsxapps/blog-next/app/layout.tsxapps/blog-nuxt/app.vueapps/blog-sveltekit/src/routes/+layout.server.tsapps/blog-sveltekit/src/routes/+layout.svelteapps/docs/docs/auth/current-auth-client.mdpackages/adapter-next/package.jsonpackages/adapter-next/src/client.tspackages/adapter-next/src/react-shim.d.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/composables/auth.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/setup.test.tspackages/adapter-nuxt/tsconfig.jsonpackages/adapter-nuxt/vitest.config.tspackages/adapter-sveltekit/package.jsonpackages/adapter-sveltekit/src/client.tspackages/adapter-sveltekit/src/server.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/src/client-runtime.tspackages/auth/src/client.tspackages/auth/src/contracts.tspackages/auth/src/index.tspackages/auth/src/runtime.tspackages/auth/tests/contracts.type.test.tspackages/auth/tests/docs-smoke.test.tspackages/auth/tests/package.test.tstests/example-app-auth-flow.mjs
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/adapter-sveltekit/tests/client.test.ts (1)
120-197:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd mocks for
@holo-js/auth/clientin theuseFormtests.The four
useFormtests (lines 120–197) don't mock@holo-js/auth/clientbefore importing../src/client, butsrc/client.tsstatically imports this module (lines 3–4). After theafterEachhook callsvi.resetModules()andvi.doUnmock('@holo-js/auth/client'), each test that doesn't explicitly callvi.doMock()will load the real auth client, which requiresfetchto be available and may fail in the test environment.Either add a shared
beforeEachhook that mocks@holo-js/auth/client:describe('@holo-js/adapter-sveltekit client', () => { + beforeEach(() => { + vi.doMock('@holo-js/auth/client', () => ({ + refreshUser: vi.fn(async () => null), + })) + }) + afterEach(() => {Or add the mock to each
useFormtest individually, following the pattern used in the earlieruseAuthtests.🤖 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-sveltekit/tests/client.test.ts` around lines 120 - 197, The failing tests load the real `@holo-js/auth/client` because src/client.ts is statically imported after vi.resetModules(); fix by mocking the auth client before importing ../src/client in these useForm tests: add a shared beforeEach that calls vi.doMock('@holo-js/auth/client', () => ({ getSession: async () => null, subscribe: () => () => {} })) (or add an equivalent vi.doMock call at the top of each affected it block) so useForm (from src/client.ts) imports the mocked auth module instead of attempting a real fetch-based client.
🧹 Nitpick comments (9)
apps/blog-next/app/auth-nav.tsx (1)
50-50: ⚡ Quick winConsider disabling the Logout button while the request is in flight.
Without an in-flight guard, rapid clicks will fire multiple concurrent
POST /api/logoutrequests. While likely idempotent server-side, it's unnecessary churn and could cause a confusing UX ifrefreshUser()is called multiple times concurrently.🛡️ Proposed fix
+ const [loggingOut, setLoggingOut] = React.useState(false) async function logout() { + if (loggingOut) return + setLoggingOut(true) try { const response = await fetch('/api/logout', { method: 'POST' }) if (!response.ok) { console.warn('Logout failed.', { status: response.status }) return } await auth.refreshUser() } catch (error) { console.warn('Logout failed.', error) + } finally { + setLoggingOut(false) } }- <button type="button" onClick={logout} style={logoutButtonStyle}>Logout</button> + <button type="button" onClick={logout} disabled={loggingOut} style={logoutButtonStyle}> + {loggingOut ? 'Logging out…' : 'Logout'} + </button>🤖 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-next/app/auth-nav.tsx` at line 50, Add an "in-flight" guard to the logout flow: introduce a local boolean state (e.g., isLoggingOut) in the AuthNav component, set it true at the start of handleLogout and false in a finally block, early-return from handleLogout if isLoggingOut is already true, and pass isLoggingOut to the Logout <button> as the disabled prop so the button is disabled while the POST /api/logout request runs; ensure refreshUser() is still called after the request and errors are caught/handled.packages/adapter-sveltekit/tests/package.test.ts (1)
18-18: 💤 Low valueConsider adding a negative assertion for
@holo-js/authinpeerDependencies.The
@holo-js/formsboundary is guarded on both sides (must be inpeerDependencies, must not be independencies). The new@holo-js/authassertion only checks the positive side — a package.json that accidentally lists@holo-js/authin bothdependenciesandpeerDependencieswould still pass.♻️ Proposed addition
expect(packageJson.dependencies?.['@holo-js/auth']).toBeDefined() + expect(packageJson.peerDependencies?.['@holo-js/auth']).toBeUndefined() expect(packageJson.dependencies?.['@holo-js/forms']).toBeUndefined()🤖 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-sveltekit/tests/package.test.ts` at line 18, The test currently asserts that packageJson.dependencies includes '@holo-js/auth' but doesn't assert it is absent from packageJson.peerDependencies; update the test in packages/adapter-sveltekit/tests/package.test.ts (near the expect(packageJson.dependencies?.['@holo-js/auth']) assertion) to also assert the negative case for peerDependencies so a package.json that lists the same package in both will fail — specifically add an assertion that packageJson.peerDependencies does not contain '@holo-js/auth' (e.g., use the inverse expect on packageJson.peerDependencies?.['@holo-js/auth']).packages/adapter-sveltekit/src/client.ts (1)
90-95: 💤 Low valueConcurrent
refreshUser()calls will overwrite with whichever response resolves last.The
AuthClientState.refreshUser()method lacks ordering guarantees—if two refresh requests are in flight, the later-resolving response wins regardless of request order.refreshCurrentUser(from@holo-js/auth/client) does not deduplicate or coalesce concurrent requests, so each call makes an independent fetch. While this race condition is unlikely in typical user-refresh scenarios, an AbortController-based guard or request coalescing would prevent stale overwrites.🤖 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-sveltekit/src/client.ts` around lines 90 - 95, The refreshUser method in AuthClientState can race when multiple calls are in-flight: replace or coalesce concurrent calls so a later-resolving fetch doesn't overwrite a newer result. Modify refreshUser to track an in-flight request token (e.g., a monotonically increasing counter or an AbortController stored on the instance) before calling refreshCurrentUser(this.requestOptions), and when the async response returns ensure the token/abort state still matches before assigning this.#user and calling this.#notify(); alternatively implement request coalescing by caching the pending Promise and returning it for concurrent callers so only one refreshCurrentUser is issued. Ensure you reference refreshUser, refreshCurrentUser, this.requestOptions, this.#user and this.#notify in the change.apps/blog-sveltekit/src/routes/+layout.svelte (2)
76-83: 💤 Low valueAdd a
:disabledstyle to reinforce the in-flight stateIf a loading guard is added to
logout()(see above), the button'sdisabledstate will have no visual differentiation from its enabled state. Consider:✨ Suggested addition
.logout-button { padding: 0; border: 0; background: transparent; color: `#cbd5e1`; cursor: pointer; font: inherit; } + .logout-button:disabled { + opacity: 0.5; + cursor: not-allowed; + }🤖 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-sveltekit/src/routes/`+layout.svelte around lines 76 - 83, Add a visual disabled state for the logout button by defining a CSS rule targeting the button's :disabled state (or the relevant button selector used in +layout.svelte) so that when logout() sets a loading/disabled flag the button visibly differs (e.g., reduced opacity, pointer-events: none, cursor: not-allowed). Update the component styles in +layout.svelte to include this :disabled styling for the logout button element and ensure any existing class-based styles don't override it.
12-25: ⚡ Quick winNo in-flight guard — concurrent logout requests on rapid clicks
The
logoutbutton has no disabled/loading state, so rapid double-clicks fire multiple concurrentPOST /api/logoutrequests. Add a simple boolean guard:🔒 Proposed fix
+ let loggingOut = $state(false) + async function logout() { + if (loggingOut) return + loggingOut = true try { const response = await fetch('/api/logout', { method: 'POST' }) if (!response.ok) { console.warn('Logout failed.', { status: response.status }) return } await auth.refreshUser() await invalidateAll() } catch (error) { console.warn('Logout failed.', error) + } finally { + loggingOut = false } }Then bind it on the button:
- <button type="button" class="logout-button" onclick={logout}>Logout</button> + <button type="button" class="logout-button" onclick={logout} disabled={loggingOut}>Logout</button>🤖 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-sveltekit/src/routes/`+layout.svelte around lines 12 - 25, Add a simple boolean guard to prevent concurrent logout requests: introduce a component-level flag (e.g., isLoggingOut) used inside the logout handler (logout function) to short-circuit if already true, set it true before starting the fetch/POST to /api/logout, await the response and in a finally block reset it to false; also bind this flag to the logout button (disabled={isLoggingOut} and/or aria-busy) so rapid clicks won't fire multiple requests. Ensure you reference the existing logout function and the logout button element when adding the guard.packages/adapter-next/tests/client.test.ts (2)
65-78: ⚡ Quick winShared
hookStatemakes multi-useStatehooks fragile.The stateful
useStatemock writes every slot into a single module-levelhookState. IfuseAuthever introduces a seconduseState(e.g. aloading/hasFetchedflag) whose setter fires after the user setter, the assertions on lines 93 and 177 (expect(hookState).toEqual(refreshedUser)) silently break even though the implementation is correct. Tracking state per slot would make these tests resilient to internal hook additions.♻️ Per-slot state suggestion
- let hookState: unknown + const hookStates: unknown[] = [] + let stateIndex = 0 @@ - useState<TValue>(initialState: TValue | (() => TValue)) { - hookState = typeof initialState === 'function' - ? (initialState as () => TValue)() - : initialState - - return [hookState as TValue, (next: TValue | ((previous: TValue) => TValue)) => { - hookState = typeof next === 'function' - ? (next as (previous: TValue) => TValue)(hookState as TValue) - : next - }] as const - }, + useState<TValue>(initialState: TValue | (() => TValue)) { + const slot = stateIndex++ + hookStates[slot] = typeof initialState === 'function' + ? (initialState as () => TValue)() + : initialState + + return [hookStates[slot] as TValue, (next: TValue | ((previous: TValue) => TValue)) => { + hookStates[slot] = typeof next === 'function' + ? (next as (previous: TValue) => TValue)(hookStates[slot] as TValue) + : next + }] as const + },Then assert against the specific slot (e.g.
expect(hookStates[0]).toEqual(refreshedUser)).🤖 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-next/tests/client.test.ts` around lines 65 - 78, The tests use a module-level hookState in the mocked useState which makes multi-useState hooks fragile; change the mock to track state per slot (e.g. replace hookState with an array hookStates and an index cursor) so each call to the mocked useState returns [hookStates[slot], setter] where the setter updates hookStates[slot]; update the mock to advance/maintain the slot index across successive useState calls and update assertions in tests to check the specific slot (for example expect(hookStates[0]).toEqual(refreshedUser)) so additional internal useState calls in useAuth won't break the assertions.
175-175: ⚡ Quick winConsider
setTimeout(resolve, 0)for improved portability if the test environment may change.
setImmediateis a Node.js global and unavailable injsdom/happy-dom. The current vitest config explicitly setsenvironment: 'node', so the code works as-is. However, usingsetTimeout(resolve, 0)orawait Promise.resolve()(twice) would make this immune to future environment changes and improve portability with minimal effort.Alternative
- await new Promise<void>(resolve => setImmediate(resolve)) + await new Promise<void>(resolve => setTimeout(resolve, 0))🤖 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-next/tests/client.test.ts` at line 175, Replace the use of the Node-only global setImmediate with a portable alternative to avoid test failures in non-Node environments: change occurrences of setImmediate(resolve) to either setTimeout(resolve, 0) or use two await Promise.resolve() ticks; update the test(s) that reference setImmediate so they call setTimeout(resolve, 0) (or perform two Promise.resolve awaits) to ensure portability across jsdom/happy-dom and node.packages/adapter-sveltekit/src/server.ts (1)
13-15: ⚡ Quick winConsider documenting or enforcing JSON-safe constraint for
HoloAuthUserfields.The
JSON.parse(JSON.stringify(user)) as HoloAuthUsercast is technically unsound—Date,Map,Set, and other non-JSON-safe types will silently lose data if added toHoloAuthUserin the future. While the currentAuthUserLikebase interface and tests only show JSON-safe primitives, the[key: string]: unknowncatch-all and extensible registry allow non-JSON fields to be added without TypeScript catching the type mismatch at the cast site.If the goal is to ensure client serialization safety, either:
- Document that
HoloAuthUseris intentionally restricted to JSON-safe primitives- Remove the unsafe cast and validate the return type explicitly
- Use
structuredClone()if deep cloning without JSON restrictions is needed (though this won't help with the type contract)Note: The shallow copy approach in
adapter-next({ ...user }) is similarly unsound but less defensive; the SvelteKit round-trip at least validates serializability at runtime.🤖 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-sveltekit/src/server.ts` around lines 13 - 15, The current toClientAuthUser function performs an unsafe JSON round-trip and casts back to HoloAuthUser, which can silently drop non-JSON-safe fields (e.g., Date, Map); to fix, either (A) enforce/document that HoloAuthUser (and its base AuthUserLike) only allows JSON-safe primitives and add a comment/ts type constraint to that effect, or (B) remove the unsafe cast in toClientAuthUser and add an explicit runtime validation that all keys are JSON-serializable before returning, or (C) if you need a deep clone that preserves non-JSON types, replace JSON.parse(JSON.stringify(user)) with structuredClone(user) and update the return type/ts definitions accordingly; update references to toClientAuthUser, HoloAuthUser, and AuthUserLike (and mention adapter-next behavior if aligning implementations) so the chosen approach is consistently applied and the type contract is enforced or documented.packages/adapter-nuxt/tests/module.test.ts (1)
339-343: 💤 Low valuePrefer
arrayContainingover positional index access for consistency withsetup.test.ts.The
[4]positional access is more fragile than the order-independent approach used insetup.test.ts(lines 425–430). If the import registration order inmodule.tsever changes, this assertion would produce a confusing failure (showing a different import at index 4 rather than a clear "useAuth not found" message).♻️ Suggested alternative
- expect(addImports.mock.calls[0]?.[0][4]).toEqual(expect.objectContaining({ - name: 'useAuth', - as: 'useAuth', - from: '@holo-js/adapter-nuxt/auth', - })) expect(addImports.mock.calls[0]?.[0]).toEqual(expect.arrayContaining([ expect.objectContaining({ name: 'holo', as: 'holo', from: '@holo-js/adapter-nuxt/runtime' }), + expect.objectContaining({ name: 'useAuth', as: 'useAuth', from: '@holo-js/adapter-nuxt/auth' }), expect.objectContaining({ name: 'useStorage', as: 'useStorage', from: '@holo-js/adapter-nuxt/storage' }), expect.objectContaining({ name: 'Storage', as: 'Storage', from: '@holo-js/adapter-nuxt/storage' }), ]))🤖 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/module.test.ts` around lines 339 - 343, The test currently asserts a specific positional entry (addImports.mock.calls[0]?.[0][4]) which is fragile; update the assertion to check that the array of imports contains an object matching name: 'useAuth', as: 'useAuth', from: '@holo-js/adapter-nuxt/auth' using an order-independent matcher (e.g., expect.arrayContaining) against addImports.mock.calls[0]?.[0] so the test fails clearly when the import is missing rather than when indices shift.
🤖 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-sveltekit/src/routes/`+layout.svelte:
- Line 9: The template accesses data.auth.user directly which can throw if
data.auth is absent; update all uses of data.auth.user in +layout.svelte to use
optional chaining (e.g., data?.auth?.user) or safe guards (e.g., conditional
checks like {`#if` data?.auth?.user}) and provide a sensible fallback where needed
so the component won’t crash when the server load omits auth; look for
references to data.auth.user in the file and replace them with the
optional-chained form and add fallback UI or null-handling as appropriate.
In `@packages/adapter-sveltekit/src/client.ts`:
- Around line 98-110: The current useAuth implementation replaces an inherited
auth context whenever any truthy options are passed; change logic so existing
context (from tryGetAuthContext) is reused unless options explicitly provide an
initialUser. Concretely: in useAuth, check if context exists and (options is
falsy OR options.initialUser === undefined) — in that case merge/assign the
derived requestOptions into the existing context (do not create a new
AuthClientState or call trySetAuthContext) and return the existing context; only
instantiate new AuthClientState and call trySetAuthContext when an explicit
initialUser is provided in UseAuthOptions (otherwise preserve inherited
initialUser and avoid shadowing). Ensure you reference useAuth,
tryGetAuthContext, trySetAuthContext, AuthClientState, initialUser, and
requestOptions when making the change.
In `@packages/adapter-sveltekit/src/server.ts`:
- Around line 17-27: auth currently lets errors from holoAuth.guard(...).user()
or currentUser() bubble up; wrap the user-resolution logic in a try/catch inside
the auth function, catch any exception from holoAuth.guard(options.guard).user()
or currentUser(), optionally log or report the error, then return a normalized
guest AuthState ({ authenticated: false, user: null }) instead of throwing; keep
using toClientAuthUser when resolution succeeds so existing behavior is
preserved.
In `@tests/example-app-auth-flow.mjs`:
- Around line 141-142: The code interpolates sessionCookieName directly into a
RegExp (where the cookie-matching regex is built, tied to sessionCookieName and
checkPages) which breaks for cookie names containing regex metacharacters; add
an escape step (e.g., an escapeRegExp helper) to quote sessionCookieName before
passing it into new RegExp so the constructed pattern matches the literal cookie
name instead of being interpreted as a regex.
---
Outside diff comments:
In `@packages/adapter-sveltekit/tests/client.test.ts`:
- Around line 120-197: The failing tests load the real `@holo-js/auth/client`
because src/client.ts is statically imported after vi.resetModules(); fix by
mocking the auth client before importing ../src/client in these useForm tests:
add a shared beforeEach that calls vi.doMock('@holo-js/auth/client', () => ({
getSession: async () => null, subscribe: () => () => {} })) (or add an
equivalent vi.doMock call at the top of each affected it block) so useForm (from
src/client.ts) imports the mocked auth module instead of attempting a real
fetch-based client.
---
Nitpick comments:
In `@apps/blog-next/app/auth-nav.tsx`:
- Line 50: Add an "in-flight" guard to the logout flow: introduce a local
boolean state (e.g., isLoggingOut) in the AuthNav component, set it true at the
start of handleLogout and false in a finally block, early-return from
handleLogout if isLoggingOut is already true, and pass isLoggingOut to the
Logout <button> as the disabled prop so the button is disabled while the POST
/api/logout request runs; ensure refreshUser() is still called after the request
and errors are caught/handled.
In `@apps/blog-sveltekit/src/routes/`+layout.svelte:
- Around line 76-83: Add a visual disabled state for the logout button by
defining a CSS rule targeting the button's :disabled state (or the relevant
button selector used in +layout.svelte) so that when logout() sets a
loading/disabled flag the button visibly differs (e.g., reduced opacity,
pointer-events: none, cursor: not-allowed). Update the component styles in
+layout.svelte to include this :disabled styling for the logout button element
and ensure any existing class-based styles don't override it.
- Around line 12-25: Add a simple boolean guard to prevent concurrent logout
requests: introduce a component-level flag (e.g., isLoggingOut) used inside the
logout handler (logout function) to short-circuit if already true, set it true
before starting the fetch/POST to /api/logout, await the response and in a
finally block reset it to false; also bind this flag to the logout button
(disabled={isLoggingOut} and/or aria-busy) so rapid clicks won't fire multiple
requests. Ensure you reference the existing logout function and the logout
button element when adding the guard.
In `@packages/adapter-next/tests/client.test.ts`:
- Around line 65-78: The tests use a module-level hookState in the mocked
useState which makes multi-useState hooks fragile; change the mock to track
state per slot (e.g. replace hookState with an array hookStates and an index
cursor) so each call to the mocked useState returns [hookStates[slot], setter]
where the setter updates hookStates[slot]; update the mock to advance/maintain
the slot index across successive useState calls and update assertions in tests
to check the specific slot (for example
expect(hookStates[0]).toEqual(refreshedUser)) so additional internal useState
calls in useAuth won't break the assertions.
- Line 175: Replace the use of the Node-only global setImmediate with a portable
alternative to avoid test failures in non-Node environments: change occurrences
of setImmediate(resolve) to either setTimeout(resolve, 0) or use two await
Promise.resolve() ticks; update the test(s) that reference setImmediate so they
call setTimeout(resolve, 0) (or perform two Promise.resolve awaits) to ensure
portability across jsdom/happy-dom and node.
In `@packages/adapter-nuxt/tests/module.test.ts`:
- Around line 339-343: The test currently asserts a specific positional entry
(addImports.mock.calls[0]?.[0][4]) which is fragile; update the assertion to
check that the array of imports contains an object matching name: 'useAuth', as:
'useAuth', from: '@holo-js/adapter-nuxt/auth' using an order-independent matcher
(e.g., expect.arrayContaining) against addImports.mock.calls[0]?.[0] so the test
fails clearly when the import is missing rather than when indices shift.
In `@packages/adapter-sveltekit/src/client.ts`:
- Around line 90-95: The refreshUser method in AuthClientState can race when
multiple calls are in-flight: replace or coalesce concurrent calls so a
later-resolving fetch doesn't overwrite a newer result. Modify refreshUser to
track an in-flight request token (e.g., a monotonically increasing counter or an
AbortController stored on the instance) before calling
refreshCurrentUser(this.requestOptions), and when the async response returns
ensure the token/abort state still matches before assigning this.#user and
calling this.#notify(); alternatively implement request coalescing by caching
the pending Promise and returning it for concurrent callers so only one
refreshCurrentUser is issued. Ensure you reference refreshUser,
refreshCurrentUser, this.requestOptions, this.#user and this.#notify in the
change.
In `@packages/adapter-sveltekit/src/server.ts`:
- Around line 13-15: The current toClientAuthUser function performs an unsafe
JSON round-trip and casts back to HoloAuthUser, which can silently drop
non-JSON-safe fields (e.g., Date, Map); to fix, either (A) enforce/document that
HoloAuthUser (and its base AuthUserLike) only allows JSON-safe primitives and
add a comment/ts type constraint to that effect, or (B) remove the unsafe cast
in toClientAuthUser and add an explicit runtime validation that all keys are
JSON-serializable before returning, or (C) if you need a deep clone that
preserves non-JSON types, replace JSON.parse(JSON.stringify(user)) with
structuredClone(user) and update the return type/ts definitions accordingly;
update references to toClientAuthUser, HoloAuthUser, and AuthUserLike (and
mention adapter-next behavior if aligning implementations) so the chosen
approach is consistently applied and the type contract is enforced or
documented.
In `@packages/adapter-sveltekit/tests/package.test.ts`:
- Line 18: The test currently asserts that packageJson.dependencies includes
'@holo-js/auth' but doesn't assert it is absent from
packageJson.peerDependencies; update the test in
packages/adapter-sveltekit/tests/package.test.ts (near the
expect(packageJson.dependencies?.['@holo-js/auth']) assertion) to also assert
the negative case for peerDependencies so a package.json that lists the same
package in both will fail — specifically add an assertion that
packageJson.peerDependencies does not contain '@holo-js/auth' (e.g., use the
inverse expect on packageJson.peerDependencies?.['@holo-js/auth']).
🪄 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: af43263d-9cb8-4f0d-a2ed-3446e18c8644
📒 Files selected for processing (19)
apps/blog-next/app/auth-nav.tsxapps/blog-next/tests/run.mjsapps/blog-nuxt/tests/run.mjsapps/blog-sveltekit/src/routes/+layout.svelteapps/blog-sveltekit/tests/run.mjspackages/adapter-next/src/client.tspackages/adapter-next/tests/client.test.tspackages/adapter-next/tests/package.test.tspackages/adapter-nuxt/src/runtime/composables/auth.tspackages/adapter-nuxt/src/runtime/shims.d.tspackages/adapter-nuxt/tests/module.test.tspackages/adapter-nuxt/tests/setup.test.tspackages/adapter-sveltekit/src/client.tspackages/adapter-sveltekit/src/server.tspackages/adapter-sveltekit/tests/adapter.type.test.tspackages/adapter-sveltekit/tests/client.test.tspackages/adapter-sveltekit/tests/package.test.tspackages/config/src/index.tstests/example-app-auth-flow.mjs
|
Review rate limit: 0/1 reviews remaining, refill in 18 minutes and 50 seconds. |
1 participant
Summary by CodeRabbit