Add unified SvelteKit config wrapper and validation fixes - #50
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 25 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements flash-cookie persistence for SvelteKit form validation failures, enabling error state to survive 303 redirects without raw JSON responses. It adds a config wrapper to manage hook paths, introduces a preprocessor for useForm reactivity, extends forms client to normalize SvelteKit action results with digest-encoded validation exceptions, and updates validation serialization with human-readable messages and compact payload encoding. ChangesSvelteKit Validation Flash & Form Integration
Nuxt Server Auto-Imports Reorganization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 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: 1
🧹 Nitpick comments (2)
packages/forms/src/internal/client.ts (1)
518-530: ⚖️ Poor tradeoffPotential race condition in concurrent form submissions.
If two forms submit concurrently, the
activeBrowserFormmodule-level state could be overwritten. The guard at line 526-528 only prevents clearing the wrong form but doesn't prevent a second submission from seeing the first form's context. Consider usingAsyncLocalStoragepattern or passing form context explicitly through the call chain.🤖 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/internal/client.ts` around lines 518 - 530, The current runWithBrowserFormElement uses module-level activeBrowserForm which allows races when two forms submit concurrently; replace this pattern with an AsyncLocalStorage-based context or explicit context passing: create an AsyncLocalStorage<BrowserFormElement> (or similar) and change runWithBrowserFormElement to call storage.run(form, callback) instead of setting activeBrowserForm, then update all places that read activeBrowserForm to use storage.getStore() (or accept a form argument) and remove the module-level activeBrowserForm; ensure the new storage is exported to consumers so concurrent submissions get isolated contexts.packages/adapter-sveltekit/src/client.ts (1)
102-104: 💤 Low valueConsider cleanup for
registeredFormson component unmount.Forms added to
registeredFormsare never removed. In SPA navigation scenarios where form components mount/unmount repeatedly, the Set could grow unboundedly. Consider returning an unregister function fromregisterFormthatuseFormcan call during cleanup.🤖 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 102 - 104, registeredForms is never cleaned up which can leak entries; modify registerForm (the function that currently adds a RegisteredForm to the registeredForms Set) to return an unregister function that removes that RegisteredForm from the Set (and tears down any per-form listeners/refs if applicable), then update useForm to call this unregister function during component unmount/cleanup; ensure the unregister removes only the exact RegisteredForm instance and guards against double-unregistering.
🤖 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/src/client.ts`:
- Around line 499-501: When handling the branch where result.type === 'failure'
in the function that returns ClientSubmitResult<TData, TSuccess>, avoid
returning undefined when parseJsonObject(result.data) yields undefined; instead,
call parseJsonObject(result.data) into a local variable, and if it's undefined
return a well-formed failure result (e.g., a ClientSubmitResult with type:
'failure' and a transport/error field indicating invalid JSON or a parsing
error) or re-throw a descriptive error; update the code paths around
parseJsonObject, result, and the ClientSubmitResult return so callers always
receive a valid result object (reference parseJsonObject and the failure branch
handling of result.type === 'failure').
---
Nitpick comments:
In `@packages/adapter-sveltekit/src/client.ts`:
- Around line 102-104: registeredForms is never cleaned up which can leak
entries; modify registerForm (the function that currently adds a RegisteredForm
to the registeredForms Set) to return an unregister function that removes that
RegisteredForm from the Set (and tears down any per-form listeners/refs if
applicable), then update useForm to call this unregister function during
component unmount/cleanup; ensure the unregister removes only the exact
RegisteredForm instance and guards against double-unregistering.
In `@packages/forms/src/internal/client.ts`:
- Around line 518-530: The current runWithBrowserFormElement uses module-level
activeBrowserForm which allows races when two forms submit concurrently; replace
this pattern with an AsyncLocalStorage-based context or explicit context
passing: create an AsyncLocalStorage<BrowserFormElement> (or similar) and change
runWithBrowserFormElement to call storage.run(form, callback) instead of setting
activeBrowserForm, then update all places that read activeBrowserForm to use
storage.getStore() (or accept a form argument) and remove the module-level
activeBrowserForm; ensure the new storage is exported to consumers so concurrent
submissions get isolated contexts.
🪄 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: 3d9cee94-7e55-4820-b56d-7a81ca3f6365
📒 Files selected for processing (35)
apps/blog-next/tests/run.mjsapps/blog-sveltekit/src/routes/admin/posts/[id]/edit/+page.svelteapps/blog-sveltekit/src/routes/admin/posts/new/+page.svelteapps/blog-sveltekit/svelte.config.jsapps/blog-sveltekit/tests/run.mjspackages/adapter-nuxt/src/module.tspackages/adapter-nuxt/src/runtime/server/auto-imports/holo.tspackages/adapter-nuxt/tests/module.test.tspackages/adapter-nuxt/tests/serverUtils.test.tspackages/adapter-nuxt/tests/setup.test.tspackages/adapter-sveltekit/package.jsonpackages/adapter-sveltekit/src/client.tspackages/adapter-sveltekit/src/config.tspackages/adapter-sveltekit/src/index.tspackages/adapter-sveltekit/src/preprocess.tspackages/adapter-sveltekit/tests/client.test.tspackages/adapter-sveltekit/tests/config.test.tspackages/adapter-sveltekit/tests/package.test.tspackages/adapter-sveltekit/tests/preprocess.test.tspackages/adapter-sveltekit/tests/runtime.test.tspackages/adapter-sveltekit/tsup.config.tspackages/auth/src/runtime/result.tspackages/auth/tests/package.test.tspackages/broadcast/tests/auth.test.tspackages/cli/src/project/registry-svelte.tspackages/cli/src/project/scaffold/framework-renderers.tspackages/cli/tests/cli.test.tspackages/forms/src/internal/client.tspackages/forms/src/internal/validation-exception.tspackages/forms/tests/client.test.tspackages/forms/tests/contracts.test.tspackages/validation/src/contracts-runtime.tspackages/validation/src/contracts-support.tspackages/validation/src/contracts.tspackages/validation/tests/contracts.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/adapter-sveltekit/tests/runtime.test.ts (2)
480-482: ⚡ Quick winEscape the cookie name in the RegExp constructor.
Constructing a regular expression from a variable without escaping can be vulnerable to ReDoS if the variable contains regex metacharacters. While the cookie name is likely controlled in this test context, using string methods is safer and sets a better pattern.
🛡️ Proposed fix using string methods instead of regex
- const flashedCookieValue = mapped.headers.get('set-cookie') - ?.match(new RegExp(`${adapterSvelteKitInternals.validationFlashCookie}=([^;]+)`))?.[1] + const setCookieHeader = mapped.headers.get('set-cookie') ?? '' + const cookiePrefix = `${adapterSvelteKitInternals.validationFlashCookie}=` + const startIndex = setCookieHeader.indexOf(cookiePrefix) + const flashedCookieValue = startIndex !== -1 + ? setCookieHeader.slice(startIndex + cookiePrefix.length).split(';')[0] + : undefined🤖 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 480 - 482, The test builds a RegExp from adapterSvelteKitInternals.validationFlashCookie which can mis-handle regex metacharacters; replace the RegExp-based extraction with a safe string-based parse: retrieve mapped.headers.get('set-cookie'), split on ';' or search for the cookie name using indexOf/startsWith, and extract the value by slicing after `${adapterSvelteKitInternals.validationFlashCookie}=`; update the reference in the test where flashedCookieValue is computed (the mapped.headers.get and adapterSvelteKitInternals.validationFlashCookie usage) so no RegExp constructor is used.
483-483: 💤 Low valueThe
'{}'fallback may be redundant after the.toBeDefined()assertion.Line 482 already asserts that
flashedCookieValueis defined, so the?? '{}'fallback on line 483 should never execute in a passing test. If you want to maintain defensive coding, this is fine, but it could also mask unexpected failures.♻️ Optional simplification
expect(flashedCookieValue).toBeDefined() -expect(JSON.parse(decodeURIComponent(flashedCookieValue ?? '{}'))).toMatchObject({ +expect(JSON.parse(decodeURIComponent(flashedCookieValue!))).toMatchObject({ values: { email: 'user@app.test', }, })🤖 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` at line 483, Remove the redundant "?? '{}'" fallback on the JSON.parse call since the previous expect(flashedCookieValue).toBeDefined() guarantees it's not null/undefined; update the assertion to use JSON.parse(decodeURIComponent(flashedCookieValue)) (or keep a defensive check if you prefer) so the test fails when flashedCookieValue is unexpectedly missing; key symbols: flashedCookieValue and the expect(JSON.parse(decodeURIComponent(...))).toMatchObject(...) assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/docs/docs/validation/index.md`:
- Line 36: The documentation has a contradiction about safeParse(...): remove
safeParse(...) from the “does not own” list and ensure it only appears in the
“owns” list for `@holo-js/validation`; update the bullet lists so that
safeParse(...) is listed alongside submission.fail() and submission.success() as
owned by `@holo-js/validation` and no longer appears in the example of server
helpers not owned by that package.
---
Nitpick comments:
In `@packages/adapter-sveltekit/tests/runtime.test.ts`:
- Around line 480-482: The test builds a RegExp from
adapterSvelteKitInternals.validationFlashCookie which can mis-handle regex
metacharacters; replace the RegExp-based extraction with a safe string-based
parse: retrieve mapped.headers.get('set-cookie'), split on ';' or search for the
cookie name using indexOf/startsWith, and extract the value by slicing after
`${adapterSvelteKitInternals.validationFlashCookie}=`; update the reference in
the test where flashedCookieValue is computed (the mapped.headers.get and
adapterSvelteKitInternals.validationFlashCookie usage) so no RegExp constructor
is used.
- Line 483: Remove the redundant "?? '{}'" fallback on the JSON.parse call since
the previous expect(flashedCookieValue).toBeDefined() guarantees it's not
null/undefined; update the assertion to use
JSON.parse(decodeURIComponent(flashedCookieValue)) (or keep a defensive check if
you prefer) so the test fails when flashedCookieValue is unexpectedly missing;
key symbols: flashedCookieValue and the
expect(JSON.parse(decodeURIComponent(...))).toMatchObject(...) assertion.
🪄 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: 3ce957b8-5cb3-4300-8ab3-0b014c26ca48
📒 Files selected for processing (23)
apps/blog-nuxt/app/pages/admin/posts/[id]/edit.vueapps/blog-nuxt/app/pages/admin/posts/new.vueapps/docs/docs/auth/current-auth-client.mdapps/docs/docs/auth/email-verification.mdapps/docs/docs/auth/index.mdapps/docs/docs/auth/local-auth.mdapps/docs/docs/forms/client-usage.mdapps/docs/docs/forms/framework-integration.mdapps/docs/docs/forms/index.mdapps/docs/docs/forms/server-validation.mdapps/docs/docs/installation.mdapps/docs/docs/media.mdapps/docs/docs/security.mdapps/docs/docs/validation/index.mdapps/docs/docs/validation/rules-and-errors.mdpackages/adapter-sveltekit/src/client.tspackages/adapter-sveltekit/src/index.tspackages/adapter-sveltekit/tests/client.test.tspackages/adapter-sveltekit/tests/runtime.test.tspackages/cli/src/project/scaffold/framework-renderers.tspackages/cli/tests/cli.test.tspackages/forms/src/internal/client.tspackages/forms/tests/client.test.ts
✅ Files skipped from review due to trivial changes (2)
- apps/docs/docs/installation.md
- apps/docs/docs/validation/rules-and-errors.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/adapter-sveltekit/src/index.ts
- packages/adapter-sveltekit/src/client.ts
- packages/cli/tests/cli.test.ts
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
Refactor
Documentation