Codex/media upload validation snapshot#48
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds comprehensive media (image) upload functionality to blog post creation and editing across Next.js, Nuxt, and SvelteKit applications. It introduces database media tables, form schema validation with file constraints, admin UI components for image selection and preview, server-side transaction-wrapped post operations with media handling, form failure recovery via cookie flashing (Nuxt), CLI scaffolding commands, and extensive test coverage for upload workflows. ChangesMedia Upload & Post Operations
Admin Form Processing & UI
Public Post Image Display
Form Infrastructure & Validation
Media Validation & Errors
CLI Media Commands
Tests & Documentation
🎯 4 (Complex) | ⏱️ ~60 minutesPossibly related PRs
✨ 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: 8
🧹 Nitpick comments (5)
apps/blog-nuxt/app/pages/admin/posts/[id]/edit.vue (1)
18-18: ⚡ Quick winUse descriptive
alttext for the current image preview.At Line 18,
alt=""hides useful context from assistive tech. A short descriptive alt (e.g., current post image) improves edit-page accessibility with minimal change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-nuxt/app/pages/admin/posts/`[id]/edit.vue at line 18, Replace the empty alt attribute on the image preview so assistive tech gets a descriptive label: update the <img> that renders when data?.imageUrl to provide a meaningful alt like “Current post image” or build a dynamic label using the post title (e.g., include data.title if present) so the preview conveys context to screen readers.apps/blog-next/tests/run.mjs (1)
45-45: ⚡ Quick winAvoid importing Next internal compiled Flight helpers in tests
apps/blog-next/tests/run.mjs:45hard-codesencodeReplyvianext/dist/compiled/react-server-dom-webpack/client.node.js. Next doesn’t provide a documented/public supported import path forencodeReply, so this is prone to break on routine Next upgrades.Prefer testing server actions through Next’s runtime behavior (e.g., issuing the same request/form submission your test is validating) instead of importing
encodeReply. If keeping this import, wrap it in atry/catchand throw a clear “unsupported Next internal path” error to make failures actionable.🤖 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/tests/run.mjs` at line 45, The test is directly importing Next internal helper encodeReply from next/dist/compiled/react-server-dom-webpack/client.node.js which is unsupported and brittle; update apps/blog-next/tests/run.mjs to avoid that import by exercising server actions via Next’s runtime (simulate the same HTTP request/form submission your test validates) instead of calling encodeReply, or if you must keep it wrap the require('next/dist/compiled/react-server-dom-webpack/client.node.js') in a try/catch and rethrow a clear error like "unsupported Next internal path: encodeReply" so failures are actionable; locate the usage of encodeReply in run.mjs and replace with runtime request logic or guarded import.apps/blog-nuxt/app/pages/posts/[slug].vue (1)
14-14: 💤 Low valueConsider moving inline styles to scoped CSS.
The inline styles could be extracted to the scoped
<style>block for better maintainability.♻️ Proposed refactor
- <img v-if="data?.imageUrl" :src="data.imageUrl" alt="" style="width: 100%; border-radius: 1rem; background: `#111827`;"> + <img v-if="data?.imageUrl" :src="data.imageUrl" alt="" class="cover-image">Then in the
<style scoped>section:<style scoped> .stack { display: grid; gap: 1rem; } .panel { padding: 1.25rem; border-radius: 1rem; background: `#111827`; line-height: 1.8; } .cover-image { width: 100%; border-radius: 1rem; background: `#111827`; } </style>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-nuxt/app/pages/posts/`[slug].vue at line 14, Remove the inline style on the image element that currently uses style="width: 100%; border-radius: 1rem; background: `#111827`;" and add a semantic class (e.g., "cover-image") to the <img v-if="data?.imageUrl" :src="data.imageUrl" alt="">; then move those style rules into the component's <style scoped> block (define .cover-image { width: 100%; border-radius: 1rem; background: `#111827`; }) and, if applicable, extract other inline styles used in this template into scoped classes like .stack and .panel to replace inline styling for maintainability.apps/blog-nuxt/server/api/admin/posts/[id].get.ts (1)
11-11: Confirm media lookup behavior:getFirstMediaUrlexists and returnsnull(not throw) for missing media/URL generation.
data.post.getFirstMediaUrl('images', 'thumb')is provided bypackages/mediaand is typed asPromise<string | null>. Its implementation returnsitem?.getUrl(conversionName) ?? null, andMediaItem.getUrl()already catches Storage URL errors and returnsnull. The remaining gap is only potential upstream DB/query failures insidegetFirstMedia()/query.first()—wrap with try/catch and fall back tonullif the admin endpoint should never fail due to media.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blog-nuxt/server/api/admin/posts/`[id].get.ts at line 11, The call to data.post.getFirstMediaUrl('images','thumb') can surface DB/query errors even though it normally returns string|null, so update the admin GET handler to guard against failures by wrapping the getFirstMediaUrl call (and upstream getFirstMedia()/query.first calls) in a try/catch and on any exception set imageUrl to null; locate the media lookup in the handler where imageUrl is assigned and change it to catch errors from getFirstMediaUrl (or its internals) and return null as a safe fallback so the admin endpoint never fails due to media lookup.apps/blog-sveltekit/src/routes/admin/posts/[id]/edit/+page.server.ts (1)
46-75: ⚖️ Poor tradeoffConsider preserving post updates even when image upload fails.
If the image upload fails (lines 46-51), the early return prevents
post.update()from executing, so all text/category/tag changes are lost. Users may expect their non-image edits to be saved even if the image fails.One option is to save post data first, then attempt the image upload outside the critical path, or allow partial success.
🤖 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/admin/posts/`[id]/edit/+page.server.ts around lines 46 - 75, The code currently aborts all updates when post.addMedia fails; instead, perform the post.update and post.tags().sync first (keep uniqueSlug(Post, ...) usage and published_at logic), then attempt the image upload with post.addMedia(data.image).toMediaCollection('images') afterwards; if the upload returns an error, do not return early—log or capture the image error and return the updated post with an image-specific error payload (use submission.fail/fail only to report the image failure while still preserving and returning the saved post data), so text/category/tag changes persist even if image upload fails.
🤖 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-nuxt/app/pages/posts/`[slug].vue:
- Line 14: The img tag currently uses an empty alt (alt="") which hides
meaningful cover images from screen readers; update the <img> in
posts/[slug].vue to set alt to a descriptive value (e.g. use data.imageAlt if
present, else fallback to data.title or a generic "Post cover image") and ensure
your post data model includes an optional imageAlt/imageAltText field so editors
can supply descriptive text; adjust any types/interfaces that define the post
payload to include this optional field and update any serializers or fixtures
that produce data.imageUrl to also provide imageAlt when available.
In `@apps/blog-sveltekit/src/routes/admin/posts/new/`+page.server.ts:
- Around line 36-69: The transaction currently returns {data:null,error} when
post.addMedia().toMediaCollection fails, which doesn't trigger a rollback;
instead, throw the error inside the DB.transaction callback (i.e., replace the
early return in the post.addMedia() error branch with throwing the error) so the
transaction will rollback, and wrap the DB.transaction(...) call in a try/catch
around the callsite that then uses submission.fail(...) and fail(...) (as
currently used) to convert the thrown error into the proper response; reference
DB.transaction, post.addMedia().toMediaCollection, Post.create, submission.fail,
and fail to locate where to change control flow.
In `@apps/blog-sveltekit/src/routes/posts/`[slug]/+page.svelte:
- Line 11: Replace the empty alt on the post image with a meaningful description
derived from the post data: when rendering the image in +page.svelte (the {`#if`
data.imageUrl} block that uses data.imageUrl), set the alt to something like the
post title or a short summary (e.g., use data.title or data.summary) so screen
readers receive descriptive text rather than an empty string.
In `@apps/blog-sveltekit/tests/run.mjs`:
- Around line 331-333: The test currently takes the first token from the
combined Set-Cookie header (csrfCookie) which can be a different cookie when
multiple cookies are set; instead, extract csrfName from the HTML (csrfName
variable) first and then iterate the full list of Set-Cookie header entries (use
response.headers.getAll / response.headers.raw()['set-cookie'] or equivalent) to
find the cookie string whose name equals csrfName, then take the token before
the first semicolon for that matching entry and assign it to csrfCookie; replace
the current single-split logic that sets csrfCookie and keep csrfName extraction
as-is.
In `@apps/docs/docs/media.md`:
- Around line 24-25: The docs use a bare "holo install media" command which is
inconsistent with the rest of the page's "npx holo ..." usage and can confuse
users without a global install; update the literal command string "holo install
media" to the executable form "npx holo install media" so all install examples
use the same npx prefix and avoid command-not-found issues.
In `@packages/adapter-nuxt/src/runtime/plugins/forms.ts`:
- Around line 150-153: serializeFormFailureCookie currently JSON-serializes the
entire FormFailurePayload (which may include user-submitted fields) and stores
it in a cookie; change it to only persist the errors subset by extracting
payload.errors (e.g., const safe = { errors: payload.errors }) and
JSON.stringify/encodeURIComponent that instead, keeping the same cookie name and
attributes; apply the same change to the other occurrence that uses
FormFailurePayload (mentioned around line 256) so only errors are stored.
In `@packages/cli/tests/cli.test.ts`:
- Around line 8578-8580: Add cleanup to unmock the module mocked with
vi.doMock('../src/media-migrations') to prevent cross-test contamination: in the
test's finally cleanup block (the same place other mocks are undone) call
vi.unmock('../src/media-migrations') or otherwise restore that module mock
(e.g., vi.resetModules()/vi.restoreAllMocks() if used consistently) so the
mocked runMediaTableCommand does not leak into later tests.
In `@packages/validation/src/contracts-types.ts`:
- Around line 74-77: The exported WebFileLike currently extends Blob but tests
and isWebFileLike expect plain structural objects (e.g., { size: 100 } or {
name, type, size }); change the WebFileLike declaration in contracts-types.ts to
a structural interface that does not extend Blob and includes the optional
properties used by isWebFileLike (name?: string, lastModified?: number, type?:
string, size?: number) so it matches runtime checks in isWebFileLike and the
test inputs; update the exported type only (keep the name WebFileLike) so
consumers and type-checking align with the function isWebFileLike and existing
tests.
---
Nitpick comments:
In `@apps/blog-next/tests/run.mjs`:
- Line 45: The test is directly importing Next internal helper encodeReply from
next/dist/compiled/react-server-dom-webpack/client.node.js which is unsupported
and brittle; update apps/blog-next/tests/run.mjs to avoid that import by
exercising server actions via Next’s runtime (simulate the same HTTP
request/form submission your test validates) instead of calling encodeReply, or
if you must keep it wrap the
require('next/dist/compiled/react-server-dom-webpack/client.node.js') in a
try/catch and rethrow a clear error like "unsupported Next internal path:
encodeReply" so failures are actionable; locate the usage of encodeReply in
run.mjs and replace with runtime request logic or guarded import.
In `@apps/blog-nuxt/app/pages/admin/posts/`[id]/edit.vue:
- Line 18: Replace the empty alt attribute on the image preview so assistive
tech gets a descriptive label: update the <img> that renders when data?.imageUrl
to provide a meaningful alt like “Current post image” or build a dynamic label
using the post title (e.g., include data.title if present) so the preview
conveys context to screen readers.
In `@apps/blog-nuxt/app/pages/posts/`[slug].vue:
- Line 14: Remove the inline style on the image element that currently uses
style="width: 100%; border-radius: 1rem; background: `#111827`;" and add a
semantic class (e.g., "cover-image") to the <img v-if="data?.imageUrl"
:src="data.imageUrl" alt="">; then move those style rules into the component's
<style scoped> block (define .cover-image { width: 100%; border-radius: 1rem;
background: `#111827`; }) and, if applicable, extract other inline styles used in
this template into scoped classes like .stack and .panel to replace inline
styling for maintainability.
In `@apps/blog-nuxt/server/api/admin/posts/`[id].get.ts:
- Line 11: The call to data.post.getFirstMediaUrl('images','thumb') can surface
DB/query errors even though it normally returns string|null, so update the admin
GET handler to guard against failures by wrapping the getFirstMediaUrl call (and
upstream getFirstMedia()/query.first calls) in a try/catch and on any exception
set imageUrl to null; locate the media lookup in the handler where imageUrl is
assigned and change it to catch errors from getFirstMediaUrl (or its internals)
and return null as a safe fallback so the admin endpoint never fails due to
media lookup.
In `@apps/blog-sveltekit/src/routes/admin/posts/`[id]/edit/+page.server.ts:
- Around line 46-75: The code currently aborts all updates when post.addMedia
fails; instead, perform the post.update and post.tags().sync first (keep
uniqueSlug(Post, ...) usage and published_at logic), then attempt the image
upload with post.addMedia(data.image).toMediaCollection('images') afterwards; if
the upload returns an error, do not return early—log or capture the image error
and return the updated post with an image-specific error payload (use
submission.fail/fail only to report the image failure while still preserving and
returning the saved post data), so text/category/tag changes persist even if
image upload fails.
🪄 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: 0bc6af2c-3c25-4455-a5f6-3c9ff012ca61
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (78)
apps/blog-next/app/admin/actions.tsapps/blog-next/app/admin/posts/[id]/edit/page.tsxapps/blog-next/app/admin/posts/new/page.tsxapps/blog-next/app/admin/posts/post-form.jsxapps/blog-next/app/posts/[slug]/page.tsxapps/blog-next/lib/schemas/blog.tsapps/blog-next/next.config.tsapps/blog-next/server/db/migrations/2026_05_26_000100_create_media_table.tsapps/blog-next/server/lib/blog.tsapps/blog-next/server/models/Post.tsapps/blog-next/tests/run.mjsapps/blog-nuxt/app/pages/admin/categories/index.vueapps/blog-nuxt/app/pages/admin/posts/[id]/edit.vueapps/blog-nuxt/app/pages/admin/posts/index.vueapps/blog-nuxt/app/pages/admin/posts/new.vueapps/blog-nuxt/app/pages/admin/tags/index.vueapps/blog-nuxt/app/pages/categories/[slug].vueapps/blog-nuxt/app/pages/index.vueapps/blog-nuxt/app/pages/posts/[slug].vueapps/blog-nuxt/app/pages/posts/index.vueapps/blog-nuxt/app/pages/tags/[slug].vueapps/blog-nuxt/package.jsonapps/blog-nuxt/server/api/admin/posts/[id].get.tsapps/blog-nuxt/server/api/blog/posts/[slug].get.tsapps/blog-nuxt/server/db/migrations/2026_05_26_000100_create_media_table.tsapps/blog-nuxt/server/lib/blog.tsapps/blog-nuxt/server/models/Post.tsapps/blog-nuxt/server/routes/admin/posts/[id]/update.post.tsapps/blog-nuxt/server/routes/admin/posts/create.post.tsapps/blog-nuxt/shared/schemas/blog.tsapps/blog-nuxt/tests/run.mjsapps/blog-sveltekit/server/db/migrations/2026_05_26_000100_create_media_table.tsapps/blog-sveltekit/server/models/Post.tsapps/blog-sveltekit/src/lib/schemas/blog.tsapps/blog-sveltekit/src/lib/server/blog.tsapps/blog-sveltekit/src/routes/admin/posts/+page.server.tsapps/blog-sveltekit/src/routes/admin/posts/+page.svelteapps/blog-sveltekit/src/routes/admin/posts/[id]/edit/+page.server.tsapps/blog-sveltekit/src/routes/admin/posts/[id]/edit/+page.svelteapps/blog-sveltekit/src/routes/admin/posts/new/+page.server.tsapps/blog-sveltekit/src/routes/admin/posts/new/+page.svelteapps/blog-sveltekit/src/routes/posts/[slug]/+page.server.tsapps/blog-sveltekit/src/routes/posts/[slug]/+page.svelteapps/blog-sveltekit/tests/blog-logic.mjsapps/blog-sveltekit/tests/run.mjsapps/docs/docs/database/commands.mdapps/docs/docs/index.mdapps/docs/docs/installation.mdapps/docs/docs/media.mdpackages/adapter-nuxt/src/module.tspackages/adapter-nuxt/src/runtime/plugins/forms.tspackages/adapter-nuxt/tests/module.test.tspackages/adapter-nuxt/tests/plugin.test.tspackages/adapter-nuxt/tests/setup.test.tspackages/cli/src/cli.tspackages/cli/src/media-migrations.tspackages/cli/src/parsing.tspackages/cli/src/project.tspackages/cli/src/project/scaffold.tspackages/cli/src/project/scaffold/dependencies.tspackages/cli/src/project/shared.tspackages/cli/src/types.tspackages/cli/tests/cli.test.tspackages/forms/src/contracts.tspackages/forms/src/internal/client.tspackages/forms/src/sensitiveInput.tspackages/forms/tests/client.test.tspackages/forms/tests/contracts.test.tspackages/forms/tests/security.test.tspackages/media/src/defineMediaModel.tspackages/media/src/index.tspackages/media/src/model/adder.tspackages/media/src/model/item.tspackages/media/tests/media.test.tspackages/validation/src/contracts-support.tspackages/validation/src/contracts-types.tspackages/validation/tests/contracts.test.tstests/example-app-auth-flow.mjs
| <article class="stack"> | ||
| <NuxtLink to="/posts">Back to posts</NuxtLink> | ||
| <h1>{{ data?.title }}</h1> | ||
| <img v-if="data?.imageUrl" :src="data.imageUrl" alt="" style="width: 100%; border-radius: 1rem; background: #111827;"> |
There was a problem hiding this comment.
Add meaningful alt text for the post cover image.
The image uses an empty alt="" attribute, which tells screen readers to skip it. Post cover images are content, not decoration, and should have descriptive alt text for accessibility. Consider adding an altText or imageAlt field to the post data, or at minimum use the post title as fallback.
♿ Proposed fix for accessibility
- <img v-if="data?.imageUrl" :src="data.imageUrl" alt="" style="width: 100%; border-radius: 1rem; background: `#111827`;">
+ <img v-if="data?.imageUrl" :src="data.imageUrl" :alt="data.imageAlt || data.title" style="width: 100%; border-radius: 1rem; background: `#111827`;">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/blog-nuxt/app/pages/posts/`[slug].vue at line 14, The img tag currently
uses an empty alt (alt="") which hides meaningful cover images from screen
readers; update the <img> in posts/[slug].vue to set alt to a descriptive value
(e.g. use data.imageAlt if present, else fallback to data.title or a generic
"Post cover image") and ensure your post data model includes an optional
imageAlt/imageAltText field so editors can supply descriptive text; adjust any
types/interfaces that define the post payload to include this optional field and
update any serializers or fixtures that produce data.imageUrl to also provide
imageAlt when available.
Comment on lines
+36
to
69
| const result = await DB.transaction(async () => { | ||
| const post = await Post.create({ | ||
| user_id: await ensureAuthorId(), | ||
| category_id: data.categoryId ? Number(data.categoryId) : null, | ||
| title: data.title.trim(), | ||
| slug: await uniqueSlug(Post, data.title), | ||
| excerpt: data.excerpt?.trim() || null, | ||
| body: data.body.trim(), | ||
| status: data.status, | ||
| published_at: data.status === 'published' ? new Date() : null, | ||
| }) | ||
|
|
||
| if (data.tagIds.length > 0) { | ||
| await post.tags().attach(data.tagIds) | ||
| } | ||
|
|
||
| if (data.image?.size) { | ||
| const { error } = await post.addMedia(data.image).toMediaCollection('images') | ||
| if (error) { | ||
| return { data: null, error } | ||
| } | ||
| } | ||
|
|
||
| if (data.tagIds.length > 0) { | ||
| await post.tags().attach(data.tagIds) | ||
| return { data: post, error: null } | ||
| }) | ||
| if (result.error) { | ||
| const failure = submission.fail({ | ||
| status: result.error.status, | ||
| errors: { | ||
| image: [result.error.message], | ||
| }, | ||
| }) | ||
| return fail(failure.status, failure) | ||
| } |
There was a problem hiding this comment.
Transaction commits even when image upload fails, causing inconsistent state.
When addMedia().toMediaCollection() fails, the code returns { data: null, error } from the transaction callback. However, this does not throw an exception, so the transaction commits normally. The post and tags are persisted, but the action returns fail(), leading the user to believe the operation failed entirely.
To rollback on image failure, throw the error instead of returning it:
Proposed fix
if (data.image?.size) {
const { error } = await post.addMedia(data.image).toMediaCollection('images')
if (error) {
- return { data: null, error }
+ throw error
}
}
- return { data: post, error: null }
+ return { data: post }
})
- if (result.error) {
+ } catch (err) {
+ if (err?.status && err?.message) {
const failure = submission.fail({
- status: result.error.status,
+ status: err.status,
errors: {
- image: [result.error.message],
+ image: [err.message],
},
})
return fail(failure.status, failure)
}
+ throw err
+ }🤖 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/admin/posts/new/`+page.server.ts around lines
36 - 69, The transaction currently returns {data:null,error} when
post.addMedia().toMediaCollection fails, which doesn't trigger a rollback;
instead, throw the error inside the DB.transaction callback (i.e., replace the
early return in the post.addMedia() error branch with throwing the error) so the
transaction will rollback, and wrap the DB.transaction(...) call in a try/catch
around the callsite that then uses submission.fail(...) and fail(...) (as
currently used) to convert the thrown error into the proper response; reference
DB.transaction, post.addMedia().toMediaCollection, Post.create, submission.fail,
and fail to locate where to change control flow.
| <article class="stack"> | ||
| <a href="/posts">Back to posts</a> | ||
| <h1>{data.post.title}</h1> | ||
| {#if data.imageUrl}<img src={data.imageUrl} alt="" style="width: 100%; border-radius: 1rem; background: #111827;">{/if} |
There was a problem hiding this comment.
Use meaningful alt text for the post image.
Line 11 sets alt="", which treats this image as decorative for screen readers. If this image conveys post content, provide a descriptive alt (for example, the post title).
Suggested fix
-{`#if` data.imageUrl}<img src={data.imageUrl} alt="" style="width: 100%; border-radius: 1rem; background: `#111827`;">{/if}
+{`#if` data.imageUrl}<img src={data.imageUrl} alt={data.post.title} style="width: 100%; border-radius: 1rem; background: `#111827`;">{/if}🤖 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/posts/`[slug]/+page.svelte at line 11, Replace
the empty alt on the post image with a meaningful description derived from the
post data: when rendering the image in +page.svelte (the {`#if` data.imageUrl}
block that uses data.imageUrl), set the alt to something like the post title or
a short summary (e.g., use data.title or data.summary) so screen readers receive
descriptive text rather than an empty string.
Comment on lines
+331
to
+333
| const setCookie = response.headers.get('set-cookie') ?? '' | ||
| const csrfCookie = setCookie.split(';', 1)[0] | ||
| const csrfName = html.match(/<input[^>]+name="([^"]+)"[^>]+value="([^"]+)"/)?.[1] |
There was a problem hiding this comment.
Extract the CSRF cookie by name, not by first Set-Cookie entry.
Line 332 currently grabs the first cookie token, which can be a non-CSRF cookie when multiple cookies are set. That can make this test intermittently fail.
Suggested fix
- const setCookie = response.headers.get('set-cookie') ?? ''
- const csrfCookie = setCookie.split(';', 1)[0]
+ const setCookie = response.headers.get('set-cookie') ?? ''
+ const csrfCookie = setCookie.match(/(?:^|,\s*)(XSRF-TOKEN=[^;,\s]+)/)?.[1] ?? ''📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const setCookie = response.headers.get('set-cookie') ?? '' | |
| const csrfCookie = setCookie.split(';', 1)[0] | |
| const csrfName = html.match(/<input[^>]+name="([^"]+)"[^>]+value="([^"]+)"/)?.[1] | |
| const setCookie = response.headers.get('set-cookie') ?? '' | |
| const csrfCookie = setCookie.match(/(?:^|,\s*)(XSRF-TOKEN=[^;,\s]+)/)?.[1] ?? '' | |
| const csrfName = html.match(/<input[^>]+name="([^"]+)"[^>]+value="([^"]+)"/)?.[1] |
🤖 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/tests/run.mjs` around lines 331 - 333, The test currently
takes the first token from the combined Set-Cookie header (csrfCookie) which can
be a different cookie when multiple cookies are set; instead, extract csrfName
from the HTML (csrfName variable) first and then iterate the full list of
Set-Cookie header entries (use response.headers.getAll /
response.headers.raw()['set-cookie'] or equivalent) to find the cookie string
whose name equals csrfName, then take the token before the first semicolon for
that matching entry and assign it to csrfCookie; replace the current
single-split logic that sets csrfCookie and keep csrfName extraction as-is.
Comment on lines
+24
to
+25
| `holo install media` creates the `media` table migration. For projects that already installed the | ||
| package manually, create only the table migration: |
There was a problem hiding this comment.
Use the same executable form for install commands.
Line 24 currently says holo install media, but the rest of this page uses npx holo .... Keeping this consistent avoids “command not found” confusion for users without a global install.
Suggested docs fix
-`holo install media` creates the `media` table migration. For projects that already installed the
+`npx holo install media` creates the `media` table migration. For projects that already installed the📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| `holo install media` creates the `media` table migration. For projects that already installed the | |
| package manually, create only the table migration: | |
| `npx holo install media` creates the `media` table migration. For projects that already installed the | |
| package manually, create only the table migration: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/docs/docs/media.md` around lines 24 - 25, The docs use a bare "holo
install media" command which is inconsistent with the rest of the page's "npx
holo ..." usage and can confuse users without a global install; update the
literal command string "holo install media" to the executable form "npx holo
install media" so all install examples use the same npx prefix and avoid
command-not-found issues.
Comment on lines
+150
to
+153
| function serializeFormFailureCookie(payload: FormFailurePayload): string { | ||
| const encoded = encodeURIComponent(JSON.stringify(payload)) | ||
| return `${FORM_FAILURE_COOKIE}=${encoded}; Path=/; Max-Age=60; HttpOnly; SameSite=Lax` | ||
| } |
There was a problem hiding this comment.
Flash only the error subset, not the entire failure payload.
The cookie currently serializes all failure fields, which can include extra user-submitted data and unnecessarily increases cookie/header size. You only render errors, so persist just that subset.
Suggested hardening patch
type FormFailurePayload = {
ok: false
status: number
valid: false
errors: Record<string, unknown>
} & Record<string, unknown>
+
+type FlashedFormFailure = {
+ errors: Record<string, unknown>
+}
-function serializeFormFailureCookie(payload: FormFailurePayload): string {
- const encoded = encodeURIComponent(JSON.stringify(payload))
+function serializeFormFailureCookie(payload: FormFailurePayload): string {
+ const flashed: FlashedFormFailure = { errors: payload.errors }
+ const encoded = encodeURIComponent(JSON.stringify(flashed))
return `${FORM_FAILURE_COOKIE}=${encoded}; Path=/; Max-Age=60; HttpOnly; SameSite=Lax`
}
-function readFlashedFailure(event: NitroEvent): FormFailurePayload | undefined {
+function readFlashedFailure(event: NitroEvent): FlashedFormFailure | undefined {
const value = readCookie(event, FORM_FAILURE_COOKIE)
if (!value) {
return undefined
}
try {
- const decoded = JSON.parse(decodeURIComponent(value)) as unknown
- return isFormFailurePayload(decoded) ? decoded : undefined
+ const decoded = JSON.parse(decodeURIComponent(value)) as unknown
+ if (!decoded || typeof decoded !== 'object') {
+ return undefined
+ }
+ const errors = (decoded as { errors?: unknown }).errors
+ return errors && typeof errors === 'object'
+ ? { errors: errors as Record<string, unknown> }
+ : undefined
} catch {
return undefined
}
}Also applies to: 256-256
🤖 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/plugins/forms.ts` around lines 150 - 153,
serializeFormFailureCookie currently JSON-serializes the entire
FormFailurePayload (which may include user-submitted fields) and stores it in a
cookie; change it to only persist the errors subset by extracting payload.errors
(e.g., const safe = { errors: payload.errors }) and
JSON.stringify/encodeURIComponent that instead, keeping the same cookie name and
attributes; apply the same change to the other occurrence that uses
FormFailurePayload (mentioned around line 256) so only errors are stored.
Comment on lines
+8578
to
+8580
| vi.doMock('../src/media-migrations', () => ({ | ||
| runMediaTableCommand: mediaTable, | ||
| })) |
There was a problem hiding this comment.
Unmock media-migrations in test cleanup to avoid cross-test contamination.
Line 8578 adds vi.doMock('../src/media-migrations', ...), but this mock is not unmocked in the finally cleanup block (unlike the other module mocks). That can leak mocked behavior into later tests.
Suggested fix
} finally {
vi.doUnmock('../src/queue')
vi.doUnmock('../src/queue-migrations')
vi.doUnmock('../src/cache-migrations')
+ vi.doUnmock('../src/media-migrations')
vi.doUnmock('../src/runtime')
vi.doUnmock('../src/generators')
vi.doUnmock('../src/broadcast')
vi.doUnmock('../src/project/scaffold')
vi.doUnmock('../src/dev')🤖 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/cli/tests/cli.test.ts` around lines 8578 - 8580, Add cleanup to
unmock the module mocked with vi.doMock('../src/media-migrations') to prevent
cross-test contamination: in the test's finally cleanup block (the same place
other mocks are undone) call vi.unmock('../src/media-migrations') or otherwise
restore that module mock (e.g., vi.resetModules()/vi.restoreAllMocks() if used
consistently) so the mocked runMediaTableCommand does not leak into later tests.
Comment on lines
+74
to
77
| export interface WebFileLike extends Blob { | ||
| readonly name?: string | ||
| readonly type?: string | ||
| readonly size?: number | ||
| readonly lastModified?: number | ||
| } |
There was a problem hiding this comment.
Align WebFileLike’s exported type with what isWebFileLike()/tests actually accept
isWebFileLike() accepts plain structural objects with name/type/size (tests validate inputs like { size: 100 } and { name, type, size }), but packages/validation/src/contracts-types.ts currently exports WebFileLike extends Blob, which over-promises Blob methods for values that may not implement them.
One low-impact fix if structural inputs are still intentional
-export interface WebFileLike extends Blob {
+export interface WebFileLike {
readonly name?: string
+ readonly type?: string
+ readonly size?: number
readonly lastModified?: number
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export interface WebFileLike extends Blob { | |
| readonly name?: string | |
| readonly type?: string | |
| readonly size?: number | |
| readonly lastModified?: number | |
| } | |
| export interface WebFileLike { | |
| readonly name?: string | |
| readonly type?: string | |
| readonly size?: number | |
| readonly lastModified?: 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/validation/src/contracts-types.ts` around lines 74 - 77, The
exported WebFileLike currently extends Blob but tests and isWebFileLike expect
plain structural objects (e.g., { size: 100 } or { name, type, size }); change
the WebFileLike declaration in contracts-types.ts to a structural interface that
does not extend Blob and includes the optional properties used by isWebFileLike
(name?: string, lastModified?: number, type?: string, size?: number) so it
matches runtime checks in isWebFileLike and the test inputs; update the exported
type only (keep the name WebFileLike) so consumers and type-checking align with
the function isWebFileLike and existing tests.
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
Documentation