fix last commit findings#20
Merged
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (9)
📝 WalkthroughWalkthroughThis PR refactors request context handling in the SvelteKit adapter from using ChangesRequest Context & Auth Runtime
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/forms/src/contracts.ts (1)
265-318:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
get-only accessor objects silently produce emptyHeaders
isHeaderAccessorObjectreturnstruefor objects with onlyget(noforEach, noentries). InnormalizeRequestHeaders, neither early-return branch fires for such an object, so execution falls through toObject.entries(input). For any non-plain class instance,Object.entriesreturns only own enumerable string properties — typically nothing — so all headers are silently dropped.Since
isRequestLikeHeadersnow returnstruefor such objects, they satisfyisRequestLikeInput, and aRequestis constructed with a missing header. The caller has no indication that headers were lost.🐛 Proposed fix — add an explicit early return for the `get`-only path
if (isHeaderAccessorObject(input)) { if (typeof input.forEach === 'function') { input.forEach((value, name) => { headers.append(name, value) }) return headers } if (typeof input.entries === 'function') { for (const [name, value] of input.entries()) { headers.append(name, value) } return headers } + + // get-only accessor: no enumeration capability, return empty headers + // rather than falling through to Object.entries on a non-plain object. + return headers }🤖 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/forms/src/contracts.ts` around lines 265 - 318, The normalizeRequestHeaders function can accept objects that pass isHeaderAccessorObject but only implement get, which cannot be iterated and currently falls through losing headers; update normalizeRequestHeaders to detect the "get-only" accessor (isHeaderAccessorObject(input) true && typeof input.forEach !== 'function' && typeof input.entries !== 'function' && typeof input.get === 'function') and handle it explicitly — either throw a clear TypeError indicating "get-only header accessor is not iterable" (so callers know headers were not provided) or convert it via a supported API if you have a way to enumerate names; reference the functions/types isHeaderAccessorObject, normalizeRequestHeaders and RequestLikeHeaders when making the change.packages/auth/src/runtime.ts (1)
61-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose
delete()on the public provider adapter contract.The runtime implementation includes and actively invokes
adapter.delete()at line 2256–2258, butAuthProviderAdapterBasedoes not declare this method. Typed consumers implementing the interface cannot properly expose the delete functionality without diverging from the contract.Proposed fix
diff --git a/packages/auth/src/contracts.ts b/packages/auth/src/contracts.ts type AuthProviderAdapterBase<TUser> = { findById(id: string | number): Promise<TUser | null> findByCredentials(credentials: Readonly<Record<string, unknown>>): Promise<TUser | null> create(input: Readonly<Record<string, unknown>>): Promise<TUser> + delete?(id: string | number): Promise<void> update?(user: TUser, input: Readonly<Record<string, unknown>>): Promise<TUser> matchesUser?(user: unknown): boolean getId(user: TUser): string | number🤖 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/auth/src/runtime.ts` around lines 61 - 66, The public adapter type ErasedAuthProviderAdapter (and the abstract/contract class AuthProviderAdapterBase) is missing the optional delete method but the runtime calls adapter.delete(); add delete?(id: string | number): Promise<void> to ErasedAuthProviderAdapter and declare the same optional async delete(id: string | number): Promise<void> signature on AuthProviderAdapterBase (or the equivalent provider base/interface used by the runtime) so typed implementations can implement delete() without breaking the contract; ensure the signature matches the runtime invocation of adapter.delete().
🧹 Nitpick comments (1)
packages/adapter-sveltekit/tests/runtime.test.ts (1)
17-65: ⚡ Quick winExtract duplicated
@holo-js/coremock factory used by both tests.The mock adapter body is duplicated almost verbatim in both tests (Line 17–65 and Line 102–150). Pulling it into a small local factory will reduce drift and simplify future updates to helper shape.
Refactor sketch
+function mockCoreWithCapture( + onCreateHelpers: (options: { + authRequest?: { + getCookie(name: string): Promise<string | undefined> + getHeader(name: string): Promise<string | undefined> + } + }) => void, +) { + vi.doMock('@holo-js/core', () => ({ + createHoloFrameworkAdapter: () => ({ + capabilities: {}, + async createProject() { return {} }, + async initializeProject() { return {} }, + createHelpers(options: { authRequest?: { getCookie(name: string): Promise<string | undefined>; getHeader(name: string): Promise<string | undefined> } }) { + onCreateHelpers(options) + return { + async getApp() { return {} }, + async getProject() { return {} }, + async getSession() { return undefined }, + async getAuth() { return undefined }, + async useConfig() { return undefined }, + async config() { return undefined }, + } + }, + async resetProject() {}, + internals: { getState() { return {} }, resolveOptions() { return {} } }, + }), + })) +}Also applies to: 102-150
🤖 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/runtime.test.ts` around lines 17 - 65, Pull the duplicated mock into a local factory function and use it in both tests: extract the object passed to vi.doMock('@holo-js/core', ...) into a helper like makeHoloCoreMock() that returns { createHoloFrameworkAdapter: () => ({ capabilities: {}, async createProject(){}, async initializeProject(){}, createHelpers(options){ capturedAuthRequest = options.authRequest; return { async getApp(){}, async getProject(){}, async getSession(){}, async getAuth(){}, async useConfig(){}, async config(){} } }, async resetProject(){}, internals: { getState(){}, resolveOptions(){} } }) } and then call vi.doMock('@holo-js/core', () => makeHoloCoreMock()) in both places; ensure capturedAuthRequest remains writable in the test scope and update any imports/uses of createHoloFrameworkAdapter, createHelpers, capturedAuthRequest to the shared factory.
🤖 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-sveltekit/tests/runtime.test.ts`:
- Around line 86-91: Ensure the test asserts capturedAuthRequest is defined
before using async matchers: add an explicit
expect(capturedAuthRequest).toBeDefined() (or equivalent) right before the lines
that call capturedAuthRequest.getCookie and capturedAuthRequest.getHeader, then
remove the optional chaining (capturedAuthRequest?.) so the subsequent await
expect(capturedAuthRequest.getCookie('session')).resolves... and await
expect(capturedAuthRequest.getHeader('x-request-id')).resolves... operate on a
guaranteed non-null object; repeat this for both the positive and undefined
assertions.
In `@packages/auth/src/runtime.ts`:
- Around line 2246-2258: In rollbackRegisteredUser, don't let a thrown
createdUser.delete() short-circuit the fallback: wrap the call to
createdUser.delete() (in the block checking 'delete' in createdUser) in a
try/catch, and if it throws attempt adapter.delete(serialized.id) inside the
catch (if adapter.delete exists); surface or rethrow the original or combined
error after attempting the adapter fallback so orphaned rows aren't left behind
if model-level deletion fails.
In `@packages/core/src/portable/holo.ts`:
- Around line 1405-1425: The code currently mutates the shared variable
currentAccessors (used by RequestAccessContext, resolveRequestContext, and
setRequestAccessors), producing a runtime-global accessor that can be
overwritten across concurrent requests; change this to be request-scoped by
replacing the mutable currentAccessors with a stable resolver that returns
per-request accessors instead of mutating shared state: update
setRequestAccessors to register a resolver function (or store accessors on the
per-request context) and modify resolveRequestContext/RequestAccessContext usage
so attachAuthRequestAccessors(context, ...) is invoked with the resolver result
on each call (or reads accessors off the passed-in context) so
getRequestCookie/getRequestHeader always use request-local accessors and never a
shared mutable variable; adjust references to currentAccessors,
resolveRequestContext, setRequestAccessors, and attachAuthRequestAccessors
accordingly.
---
Outside diff comments:
In `@packages/auth/src/runtime.ts`:
- Around line 61-66: The public adapter type ErasedAuthProviderAdapter (and the
abstract/contract class AuthProviderAdapterBase) is missing the optional delete
method but the runtime calls adapter.delete(); add delete?(id: string | number):
Promise<void> to ErasedAuthProviderAdapter and declare the same optional async
delete(id: string | number): Promise<void> signature on AuthProviderAdapterBase
(or the equivalent provider base/interface used by the runtime) so typed
implementations can implement delete() without breaking the contract; ensure the
signature matches the runtime invocation of adapter.delete().
In `@packages/forms/src/contracts.ts`:
- Around line 265-318: The normalizeRequestHeaders function can accept objects
that pass isHeaderAccessorObject but only implement get, which cannot be
iterated and currently falls through losing headers; update
normalizeRequestHeaders to detect the "get-only" accessor
(isHeaderAccessorObject(input) true && typeof input.forEach !== 'function' &&
typeof input.entries !== 'function' && typeof input.get === 'function') and
handle it explicitly — either throw a clear TypeError indicating "get-only
header accessor is not iterable" (so callers know headers were not provided) or
convert it via a supported API if you have a way to enumerate names; reference
the functions/types isHeaderAccessorObject, normalizeRequestHeaders and
RequestLikeHeaders when making the change.
---
Nitpick comments:
In `@packages/adapter-sveltekit/tests/runtime.test.ts`:
- Around line 17-65: Pull the duplicated mock into a local factory function and
use it in both tests: extract the object passed to vi.doMock('@holo-js/core',
...) into a helper like makeHoloCoreMock() that returns {
createHoloFrameworkAdapter: () => ({ capabilities: {}, async createProject(){},
async initializeProject(){}, createHelpers(options){ capturedAuthRequest =
options.authRequest; return { async getApp(){}, async getProject(){}, async
getSession(){}, async getAuth(){}, async useConfig(){}, async config(){} } },
async resetProject(){}, internals: { getState(){}, resolveOptions(){} } }) } and
then call vi.doMock('@holo-js/core', () => makeHoloCoreMock()) in both places;
ensure capturedAuthRequest remains writable in the test scope and update any
imports/uses of createHoloFrameworkAdapter, createHelpers, capturedAuthRequest
to the shared factory.
🪄 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: d1394675-b9a3-4b7e-85f4-902d81d1a659
📒 Files selected for processing (17)
packages/adapter-nuxt/tests/module.test.tspackages/adapter-nuxt/tests/setup.test.tspackages/adapter-sveltekit/src/index.tspackages/adapter-sveltekit/src/sveltekit.d.tspackages/adapter-sveltekit/tests/adapter.test.tspackages/adapter-sveltekit/tests/adapter.type.test.tspackages/adapter-sveltekit/tests/runtime.test.tspackages/auth/src/contracts.tspackages/auth/src/runtime.tspackages/auth/tests/package.test.tspackages/cli/src/project/registry-svelte.tspackages/cli/tests/cli.test.tspackages/core/src/portable/holo.tspackages/core/tests/auth-runtime.test.tspackages/forms/src/contracts.tspackages/forms/src/sensitiveInput.tspackages/forms/tests/contracts.test.ts
💤 Files with no reviewable changes (3)
- packages/forms/src/sensitiveInput.ts
- packages/adapter-sveltekit/src/sveltekit.d.ts
- packages/adapter-sveltekit/tests/adapter.test.ts
Merged
This was referenced May 6, 2026
Merged
Merged
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
Release Notes
New Features
Improvements