Refactor db related packages#43
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughConverts app auth to server actions/forms (Next) and SvelteKit page actions; updates UI to submit via actions/forms; revises tests and docs; and applies broad refactors across auth/forms/security/db/queue/storage/mail/core. ChangesServer-first auth and platform refactors
Sequence Diagram(s)sequenceDiagram
participant User
participant Page/Form
participant ServerAction
participant AuthLib
participant Cache
participant Redirect
User->>Page/Form: Submit credentials
Page/Form->>ServerAction: FormData
ServerAction->>AuthLib: validate + login/register/logout
AuthLib-->>ServerAction: result/session
ServerAction->>Cache: revalidatePath('/', 'layout')
ServerAction->>Redirect: redirect('/admin' | '/verify-email' | '/')
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
apps/blog-next/app/login/actions.ts (1)
20-26: Confirmlogin()failure shape matcheserror.status/error.fields(apps/blog-next/app/login/actions.ts:20-26).
@holo-js/auth’slogin()returns anAuthResult; on failure,erroris anAuthFailurethat includesstatus: numberandfields, matching the code’ssubmission.fail({ status: error.status, errors: error.fields }).
Also,login()only converts expectedAuthLoginErrorCodes into an{ error }result—other/unexpected errors will still throw (addtry/catchonly if you need that path handled).🤖 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/login/actions.ts` around lines 20 - 26, The review asks you to confirm that the shape returned by login() matches the usage: ensure the imported login() (and its types AuthResult/AuthFailure from `@holo-js/auth`) actually return an AuthFailure with status:number and fields when failing; if they do, leave the submission.fail({ status: error.status, errors: error.fields }) as-is, otherwise map the returned error shape to submission.fail (e.g., extract/convert to {status, fields}) and only add a try/catch around await login(submission.data) if you need to handle unexpected thrown exceptions; reference login(), AuthResult/AuthFailure and submission.fail when locating the code to update.packages/cache-redis/package.json (1)
24-24: ⚡ Quick winIntegration mode is gated by Vitest config (
HOLO_REDIS_INTEGRATION), not by per-test checks.
packages/cache-redis/vitest.config.tsswitchestest.includebased onprocess.env.HOLO_REDIS_INTEGRATION === '1': when off it runs onlytests/package.test.ts, and when on it runstests/**/*.test.ts(so bothtests/package.test.tsandtests/real-redis.test.ts).
Iftest:integrationshould run only the real Redis test, narrow theincludepattern accordingly.🤖 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/cache-redis/package.json` at line 24, The current Vitest gating in packages/cache-redis/vitest.config.ts uses process.env.HOLO_REDIS_INTEGRATION === '1' to switch test.include between a broad pattern and a single package.test.ts, which means running npm run test:integration still runs package.test.ts as well; update the logic in vitest.config.ts so that when HOLO_REDIS_INTEGRATION === '1' the test.include is narrowed to only the integration test (e.g., "tests/real-redis.test.ts") and when not set it remains the default (e.g., "tests/package.test.ts"); ensure the package.json script ("test:integration": "HOLO_REDIS_INTEGRATION=1 vitest --run") remains unchanged so it triggers the new narrowed include.packages/db/src/schema/generatedNames.ts (1)
4-9: 💤 Low valueConsider potential index name collisions with underscore-containing column names.
The current implementation joins column names with
'_', which could theoretically create identical names for different column combinations (e.g., columns['first_name', 'last']and['first', 'name_last']would both generatetable_first_name_last_index). However, since users can explicitly provideindex.nameto override auto-generation, this is an acceptable tradeoff for convenience.🤖 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/db/src/schema/generatedNames.ts` around lines 4 - 9, resolveGeneratedIndexName currently concatenates index.columns with '_' which can collide when column names themselves contain underscores; update resolveGeneratedIndexName to produce collision-resistant auto names by encoding the columns portion (for example, join with an unambiguous delimiter or append a short deterministic hash of index.columns) while preserving use of sanitizeIdentifierForGeneratedName and validating with assertValidIdentifierSegment; ensure the new scheme still respects an explicit index.name override and produces stable names for the same columns.packages/queue-db/tests/failed.test.ts (1)
193-227: ⚡ Quick winRestore the
DB.connectionspies in afinallyblock.If either test fails before the last line, the spy survives into later cases and obscures the real failure.
♻️ Suggested pattern
- const spy = vi.spyOn(DB, 'connection').mockReturnValue(createQueueDatabaseContextMock()) + const spy = vi.spyOn(DB, 'connection').mockReturnValue(createQueueDatabaseContextMock()) + try { configureQueueRuntime({ config: { default: 'database', @@ await expect(forgetFailedQueueJob('missing')).resolves.toBe(false) await expect(flushFailedQueueJobs()).resolves.toBe(0) - - spy.mockRestore() + } finally { + spy.mockRestore() + }Apply the same pattern to the active-async-context test below as well.
Also applies to: 277-317
🤖 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/queue-db/tests/failed.test.ts` around lines 193 - 227, The DB.connection spy created with vi.spyOn(DB, 'connection') should be restored in a finally block to guarantee cleanup even if assertions fail: wrap the test logic that calls configureQueueRuntime, persistFailedQueueJob, forgetFailedQueueJob, and flushFailedQueueJobs in a try { ... } finally { spy.mockRestore() } block (and apply the identical try/finally pattern to the active-async-context test that also creates a DB.connection spy) so the spy is always restored.
🤖 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/register/`+page.server.ts:
- Around line 9-18: The cookie `secure` flag is being set using url.protocol ===
'https:' in the load function (export const load) which ignores TLS-terminating
proxies and X-Forwarded-Proto; update the logic to detect TLS from forwarded
headers (check request.headers['x-forwarded-proto'] and/or the Forwarded header)
or, better, centralize this behavior in the security package by
implementing/using isSecureRequest(request) in `@holo-js/security` (currently used
in packages/security/src/csrf.ts) and replace the url.protocol check in the load
handler that calls csrf.field and cookies.set (and any similar uses like
getSecurityRuntime().config.csrf.cookie) so the cookie.secure is true when the
proxied request was originally HTTPS.
In `@apps/blog-sveltekit/src/routes/super-admin/login/`+page.svelte:
- Line 49: The checkbox currently uses a strict equality check
(checked={values.remember === true}) which loses checked state for non-boolean
form returns like "on"; update the checked logic for the remember checkbox
(reference: values.remember / the checked prop on the input in +page.svelte) to
accept truthy/string values instead—e.g., coerce to a boolean (use
Boolean(values.remember)) or explicitly accept "on" in addition to true—so the
checkbox remains checked after failed submissions that return string-like
values.
In `@packages/core/src/runtimeModule.ts`:
- Around line 67-69: The current optional-error handling in
importOptionalRuntimeModule treats any ERR_MODULE_NOT_FOUND for relative
specifiers (specifier.startsWith('.')) as ignorable even when
matchesRequestedTarget is false, which can hide missing transitive dependencies;
update the conditional that currently ORs ('code' in error && ... code ===
'ERR_MODULE_NOT_FOUND' && specifier.startsWith('.')) so it also requires
matchesRequestedTarget (i.e., only treat relative ERR_MODULE_NOT_FOUND as
optional when matchesRequestedTarget is true), thereby tightening the check in
the optional-error helper and the importOptionalRuntimeModule logic.
In `@packages/forms/src/contracts.ts`:
- Around line 401-410: In resolveAmbientRequestUrl, treat the Referer header as
untrusted: attempt to parse headers.get('referer') with the URL constructor (or
otherwise validate it) and only return it if parsing succeeds and the result is
a well-formed absolute URL; if parsing throws or the referer is invalid, fall
back to building the synthetic URL from x-forwarded-proto / x-forwarded-host /
host as currently done. Update resolveAmbientRequestUrl to catch URL parsing
errors (or validate before usage) and use the forwarded host/proto fallback when
referer is malformed to avoid letting a bad client header cause new Request(...)
to throw.
In `@packages/queue-db/src/index.ts`:
- Around line 1-9: Restore the removed root re-exports to preserve source
compatibility: re-add exports for queueDatabaseInternals,
databaseQueueDriverInternals, and queueDbFailedStoreInternals alongside the
current exports so consumers of the package root keep working. Locate where
databaseQueueDriverFactory/DatabaseQueueDriver/DatabaseQueueDriverError,
queueDbFailedJobStore, and the Stored* types are exported and add matching
re-export lines for the three internal symbols (queueDatabaseInternals,
databaseQueueDriverInternals, queueDbFailedStoreInternals) pointing to their
original modules so the public surface remains unchanged until a breaking
release.
---
Nitpick comments:
In `@apps/blog-next/app/login/actions.ts`:
- Around line 20-26: The review asks you to confirm that the shape returned by
login() matches the usage: ensure the imported login() (and its types
AuthResult/AuthFailure from `@holo-js/auth`) actually return an AuthFailure with
status:number and fields when failing; if they do, leave the submission.fail({
status: error.status, errors: error.fields }) as-is, otherwise map the returned
error shape to submission.fail (e.g., extract/convert to {status, fields}) and
only add a try/catch around await login(submission.data) if you need to handle
unexpected thrown exceptions; reference login(), AuthResult/AuthFailure and
submission.fail when locating the code to update.
In `@packages/cache-redis/package.json`:
- Line 24: The current Vitest gating in packages/cache-redis/vitest.config.ts
uses process.env.HOLO_REDIS_INTEGRATION === '1' to switch test.include between a
broad pattern and a single package.test.ts, which means running npm run
test:integration still runs package.test.ts as well; update the logic in
vitest.config.ts so that when HOLO_REDIS_INTEGRATION === '1' the test.include is
narrowed to only the integration test (e.g., "tests/real-redis.test.ts") and
when not set it remains the default (e.g., "tests/package.test.ts"); ensure the
package.json script ("test:integration": "HOLO_REDIS_INTEGRATION=1 vitest
--run") remains unchanged so it triggers the new narrowed include.
In `@packages/db/src/schema/generatedNames.ts`:
- Around line 4-9: resolveGeneratedIndexName currently concatenates
index.columns with '_' which can collide when column names themselves contain
underscores; update resolveGeneratedIndexName to produce collision-resistant
auto names by encoding the columns portion (for example, join with an
unambiguous delimiter or append a short deterministic hash of index.columns)
while preserving use of sanitizeIdentifierForGeneratedName and validating with
assertValidIdentifierSegment; ensure the new scheme still respects an explicit
index.name override and produces stable names for the same columns.
In `@packages/queue-db/tests/failed.test.ts`:
- Around line 193-227: The DB.connection spy created with vi.spyOn(DB,
'connection') should be restored in a finally block to guarantee cleanup even if
assertions fail: wrap the test logic that calls configureQueueRuntime,
persistFailedQueueJob, forgetFailedQueueJob, and flushFailedQueueJobs in a try {
... } finally { spy.mockRestore() } block (and apply the identical try/finally
pattern to the active-async-context test that also creates a DB.connection spy)
so the spy is always restored.
🪄 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: 97911b50-3889-4b80-9fdb-73140b3caf90
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (119)
apps/blog-next/app/auth-nav.tsxapps/blog-next/app/login/actions.tsapps/blog-next/app/login/page.tsxapps/blog-next/app/logout/actions.tsapps/blog-next/app/register/actions.tsapps/blog-next/app/register/page.tsxapps/blog-next/app/super-admin/login/actions.tsapps/blog-next/app/super-admin/login/page.tsxapps/blog-next/app/super-admin/logout-button.tsxapps/blog-next/app/super-admin/logout/actions.tsapps/blog-next/tests/auth-nav.test.mjsapps/blog-next/tests/login-page.test.mjsapps/blog-next/tests/logout-actions.test.mjsapps/blog-next/tests/register-page.test.mjsapps/blog-next/tests/run.mjsapps/blog-next/tests/super-admin-login-page.test.mjsapps/blog-next/tests/super-admin-logout-button.test.mjsapps/blog-sveltekit/src/routes/+layout.svelteapps/blog-sveltekit/src/routes/login/+page.server.tsapps/blog-sveltekit/src/routes/login/+page.svelteapps/blog-sveltekit/src/routes/logout/+server.tsapps/blog-sveltekit/src/routes/register/+page.server.tsapps/blog-sveltekit/src/routes/register/+page.svelteapps/blog-sveltekit/src/routes/super-admin/+page.server.tsapps/blog-sveltekit/src/routes/super-admin/+page.svelteapps/blog-sveltekit/src/routes/super-admin/login/+page.server.tsapps/blog-sveltekit/src/routes/super-admin/login/+page.svelteapps/blog-sveltekit/tests/auth-page-actions.test.mjsapps/blog-sveltekit/tests/run.mjsapps/docs/docs/auth/current-auth-client.mdapps/docs/docs/forms/framework-integration.mdapps/docs/docs/forms/server-validation.mdpackages/auth-social-discord/package.jsonpackages/auth-social-github/package.jsonpackages/auth-social-github/src/index.tspackages/auth-social-github/tests/package.test.tspackages/auth/src/next-server-shim.d.tspackages/auth/src/next/client.tspackages/auth/src/next/server.tspackages/auth/src/runtime/csrfCookie.tspackages/auth/tests/framework.test.tspackages/auth/tsup.config.tspackages/cache-db/tests/package.test.tspackages/cache-redis/package.jsonpackages/cache-redis/tests/package.test.tspackages/cli/package.jsonpackages/cli/tests/vitest-config.test.tspackages/config/src/access.tspackages/config/src/loader.tspackages/config/tests/broadcast-config.type.test.tspackages/config/tests/config.type.test.tspackages/config/tests/security-config.type.test.tspackages/config/tests/support/configAccessors.tspackages/core/src/portable/holo.tspackages/core/src/runtimeModule.tspackages/core/src/storageRuntime.tspackages/core/tests/dbRuntime.test.tspackages/core/tests/runtime.test.tspackages/core/tests/storageRuntime.test.tspackages/db-mysql/package.jsonpackages/db-mysql/src/index.tspackages/db-mysql/tests/mysql.test.tspackages/db-postgres/package.jsonpackages/db-postgres/src/index.tspackages/db-postgres/tests/postgres.test.tspackages/db-sqlite/src/index.tspackages/db/package.jsonpackages/db/src/cache.tspackages/db/src/core/QueryScheduler.tspackages/db/src/drivers/MySQLAdapter.tspackages/db/src/drivers/PostgresAdapter.tspackages/db/src/drivers/SQLiteAdapter.tspackages/db/src/drivers/index.tspackages/db/src/drivers/savepoints.tspackages/db/src/migrations/MigrationService.tspackages/db/src/migrations/defineMigration.tspackages/db/src/migrations/template.tspackages/db/src/model/Entity.tspackages/db/src/model/ModelQueryBuilder.tspackages/db/src/model/collection.tspackages/db/src/model/defineModel.tspackages/db/src/model/relations.tspackages/db/src/model/staticModelApi.tspackages/db/src/model/types.tspackages/db/src/query/MySQLQueryCompiler.tspackages/db/src/query/SQLiteQueryCompiler.impl.tspackages/db/src/query/paginator.tspackages/db/src/runtime.tspackages/db/src/schema/SQLSchemaCompiler.tspackages/db/src/schema/SchemaService.tspackages/db/src/schema/TableDefinitionBuilder.tspackages/db/src/schema/TableMutationBuilder.tspackages/db/src/schema/diff.tspackages/db/src/schema/foreignKeyBuilderState.tspackages/db/src/schema/generatedNames.tspackages/db/src/schema/typeMapping.tspackages/db/src/security/policy.tspackages/db/tests/core-runtime.test.tspackages/db/tests/drivers-core.test.tspackages/db/tests/factories-core.test.tspackages/db/vitest.config.tspackages/forms/src/contracts.tspackages/forms/tests/contracts.test.tspackages/forms/tsup.config.tspackages/mail/src/contracts.tspackages/mail/src/index.tspackages/mail/src/runtime.tspackages/mail/tests/contracts.test.tspackages/mail/tests/runtime.test.tspackages/queue-db/src/database.tspackages/queue-db/src/drivers/database.tspackages/queue-db/src/failed.tspackages/queue-db/src/index.tspackages/queue-db/tests/database-driver.test.tspackages/queue-db/tests/failed.test.tspackages/queue-db/tests/support/dialect.tspackages/queue-db/tests/support/sqlite-queue.tspackages/storage-s3/src/index.tspackages/storage/src/runtime/composables/index.ts
💤 Files with no reviewable changes (9)
- packages/db/src/drivers/SQLiteAdapter.ts
- packages/db/src/drivers/PostgresAdapter.ts
- packages/mail/src/contracts.ts
- packages/mail/src/index.ts
- packages/db/tests/drivers-core.test.ts
- packages/db/vitest.config.ts
- packages/db/src/drivers/MySQLAdapter.ts
- packages/db/package.json
- packages/db/tests/factories-core.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/super-admin/login/`+page.svelte:
- Around line 8-11: The login form instantiation (useForm(loginForm, { ... }))
currently omits CSRF protection; add csrf: true to the options passed to useForm
(alongside validateOn and initialValues) so the frontend includes CSRF tokens,
and update the paired submit handler/endpoint that validates the login (the
async submitter and its server-side login endpoint) to enforce CSRF validation
(reject when CSRF is missing/invalid) to match other auth flows; reference the
useForm call and the async submitter function to locate both client and server
changes.
In `@apps/docs/docs/forms/server-validation.md`:
- Around line 249-251: The Nuxt flow prose ("refresh the current user, then
navigate") is inconsistent with the Nuxt example which only refreshes and shows
a success message; either update the prose to describe "refresh and show success
message" or modify the Nuxt example to perform navigation after the refresh. To
fix, edit the docs text around the Nuxt flow or update the Nuxt snippet to call
the router navigation step (e.g., invoke the router push/redirect after the user
refresh completes) so the example matches the described flow; ensure the change
references the Nuxt snippet/example and the flow description in the same
section.
🪄 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: d8ff87ae-431c-4e4f-acdc-abc348fe5f7a
📒 Files selected for processing (30)
apps/blog-sveltekit/src/routes/api/login/+server.tsapps/blog-sveltekit/src/routes/api/register/+server.tsapps/blog-sveltekit/src/routes/api/super-admin/login/+server.tsapps/blog-sveltekit/src/routes/login/+page.svelteapps/blog-sveltekit/src/routes/register/+page.svelteapps/blog-sveltekit/src/routes/super-admin/login/+page.svelteapps/blog-sveltekit/tests/auth-page-actions.test.mjsapps/docs/docs/auth/current-auth-client.mdapps/docs/docs/forms/framework-integration.mdapps/docs/docs/forms/server-validation.mdapps/docs/docs/security.mdpackages/auth/src/next/server.tspackages/auth/src/runtime/csrfCookie.tspackages/auth/src/sveltekit/server.tspackages/auth/tests/framework.test.tspackages/cache-redis/vitest.config.tspackages/core/src/runtimeModule.tspackages/core/tests/runtimeModule.test.tspackages/db/src/schema/generatedNames.tspackages/db/tests/schema-service.test.tspackages/forms/src/contracts.tspackages/forms/tests/contracts.test.tspackages/queue-db/src/database.tspackages/queue-db/src/drivers/database.tspackages/queue-db/src/failed.tspackages/queue-db/src/index.tspackages/queue-db/tests/failed.test.tspackages/security/src/csrf.tspackages/security/src/index.tspackages/security/tests/package.test.ts
✅ Files skipped from review due to trivial changes (3)
- apps/blog-sveltekit/src/routes/api/register/+server.ts
- apps/docs/docs/security.md
- packages/db/tests/schema-service.test.ts
Comment on lines
8
to
11
| const form = useForm(loginForm, { | ||
| validateOn: 'blur', | ||
| initialValues: { email: '', password: '', remember: false }, | ||
| async submitter({ formData }) { |
There was a problem hiding this comment.
Re-enable CSRF protection for the super-admin login flow.
Line 8 config omits csrf: true, and the paired endpoint currently validates without CSRF as well. That leaves this login route weaker than the other auth entry points in this PR.
🔧 Proposed fix
// apps/blog-sveltekit/src/routes/super-admin/login/+page.svelte
const form = useForm(loginForm, {
+ csrf: true,
validateOn: 'blur',
initialValues: { email: '', password: '', remember: false },// apps/blog-sveltekit/src/routes/api/super-admin/login/+server.ts
const submission = await validate(request, loginForm, {
+ csrf: true,
throttle: 'login',
})🤖 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/super-admin/login/`+page.svelte around lines 8
- 11, The login form instantiation (useForm(loginForm, { ... })) currently omits
CSRF protection; add csrf: true to the options passed to useForm (alongside
validateOn and initialValues) so the frontend includes CSRF tokens, and update
the paired submit handler/endpoint that validates the login (the async submitter
and its server-side login endpoint) to enforce CSRF validation (reject when CSRF
is missing/invalid) to match other auth flows; reference the useForm call and
the async submitter function to locate both client and server changes.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/src/schema/generatedNames.ts (1)
47-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSanitize
columnNamein fallback foreign-key name generation.The fallback path sanitizes
tableNamebut notcolumnName, so non-identifier-safe column names can fail generated-name validation unexpectedly.Proposed fix
- const resolvedName = constraintName ?? `${sanitizeIdentifierForGeneratedName(tableName)}_${columnName}_foreign` + const resolvedName = constraintName + ?? `${sanitizeIdentifierForGeneratedName(tableName)}_${sanitizeIdentifierForGeneratedName(columnName)}_foreign`🤖 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/db/src/schema/generatedNames.ts` around lines 47 - 48, The fallback foreign-key name uses sanitizeIdentifierForGeneratedName on tableName but not columnName, which can break assertValidIdentifierSegment; update the resolvedName construction in the block that defines resolvedName (using constraintName ?? ...) to sanitize columnName as well (e.g., call sanitizeIdentifierForGeneratedName(columnName) when building the `${...}_${...}_foreign` fallback) so that both tableName and columnName are identifier-safe before assertValidIdentifierSegment is invoked.
🤖 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/forms/server-validation.md`:
- Line 246: Change the heading text "Full page flow" to use hyphenated compound
wording "Full-page flow" in the docs file—update the heading string where it
appears as the section title "Full page flow" to "Full-page flow" so it follows
standard compound-word style.
In `@tests/example-app-auth-flow.mjs`:
- Around line 348-399: The current action-mode helper swallows structured action
failure payloads and assertAuthFieldFailure ignores the provided fields when
usesSvelteKitActions is true; update the action-handling block in
fetchActionSubmission (the try/catch that parses result.text) to preserve and
return the parsed actionResult (including actionResult.type === 'failure' and
actionResult.errors) instead of always returning the generic "_root" error, and
modify assertAuthFieldFailure to, when usesSvelteKitActions is true, delegate to
assertFieldFailure by passing the preserved result.json.errors (or the same
shape assertFieldFailure expects) so field-level assertions (e.g., ['email'])
are validated; reference functions: fetchActionSubmission,
assertAuthFieldFailure, and assertFieldFailure.
---
Outside diff comments:
In `@packages/db/src/schema/generatedNames.ts`:
- Around line 47-48: The fallback foreign-key name uses
sanitizeIdentifierForGeneratedName on tableName but not columnName, which can
break assertValidIdentifierSegment; update the resolvedName construction in the
block that defines resolvedName (using constraintName ?? ...) to sanitize
columnName as well (e.g., call sanitizeIdentifierForGeneratedName(columnName)
when building the `${...}_${...}_foreign` fallback) so that both tableName and
columnName are identifier-safe before assertValidIdentifierSegment is invoked.
🪄 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: d0703608-cc8d-41b1-8aa2-2f62d43c3502
📒 Files selected for processing (30)
apps/blog-sveltekit/src/routes/+layout.server.tsapps/blog-sveltekit/src/routes/api/login/+server.tsapps/blog-sveltekit/src/routes/api/register/+server.tsapps/blog-sveltekit/src/routes/api/super-admin/login/+server.tsapps/blog-sveltekit/src/routes/login/+page.server.tsapps/blog-sveltekit/src/routes/login/+page.svelteapps/blog-sveltekit/src/routes/register/+page.server.tsapps/blog-sveltekit/src/routes/register/+page.svelteapps/blog-sveltekit/src/routes/super-admin/login/+page.server.tsapps/blog-sveltekit/src/routes/super-admin/login/+page.svelteapps/blog-sveltekit/tests/auth-page-actions.test.mjsapps/blog-sveltekit/tests/blog-logic.mjsapps/blog-sveltekit/tests/register-route.test.mjsapps/blog-sveltekit/tests/run.mjsapps/docs/docs/forms/client-usage.mdapps/docs/docs/forms/framework-integration.mdapps/docs/docs/forms/server-validation.mdpackages/adapter-sveltekit/src/client.tspackages/adapter-sveltekit/src/sveltekit-app.d.tspackages/adapter-sveltekit/tests/client.test.tspackages/adapter-sveltekit/tests/stubs/app-stores.tspackages/adapter-sveltekit/tsup.config.tspackages/adapter-sveltekit/vitest.config.tspackages/auth/src/sveltekit/server.tspackages/db/src/schema/generatedNames.tspackages/db/tests/schema-service.test.tspackages/forms/src/internal/client.tspackages/forms/tests/client.test.tstests/example-app-auth-flow.mjstests/example-app-token-auth-flow.mjs
💤 Files with no reviewable changes (4)
- apps/blog-sveltekit/tests/register-route.test.mjs
- apps/blog-sveltekit/src/routes/api/login/+server.ts
- apps/blog-sveltekit/src/routes/api/register/+server.ts
- apps/blog-sveltekit/src/routes/api/super-admin/login/+server.ts
✅ Files skipped from review due to trivial changes (3)
- packages/adapter-sveltekit/tsup.config.ts
- packages/adapter-sveltekit/tests/stubs/app-stores.ts
- packages/adapter-sveltekit/src/sveltekit-app.d.ts
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes