feat(recipes): image upload, fix save 500, HelloFresh seed data #53
Reference in New Issue
Block a user
Delete Branch "feat/issue-46-wire-suggestions-recipe-picker"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
textDB column via V023 migration)serves/cookTimeMinchanged from primitiveshortto nullableInteger,isChildFriendlyremoved from the request DTO (no form field)tagType === 'category'filter; tags now grouped by type with German headings (Ernährung, Küche, Protein, Sonstiges)SuggestionResponse.SuggestionRecipe(no image) fromSlotResponse.SlotRecipeso suggestion payloads stay lightweight- Suggestion interface: { recipe, scoreDelta, hasConflict } (no simulatedScore) - Badge renders from hasConflict directly — no client-side delta computation needed - New isLoading prop shows skeleton rows while suggestions fetch is in flight - currentVarietyScore prop removed from component and both call sites follow in next commit Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Blockers
1.
IngredientEntry.quantityvalidation vs. frontend fallback mismatchRecipeCreateRequest.IngredientEntrystill has@DecimalMin("0.01")but the frontend sends0as a fallback for empty quantity fields (Number(ing.quantity) || 0). If a user leaves quantity blank, the backend will return a 400 validation error that surfaces as a generic "Fehler beim Speichern" — silent failure with no actionable feedback.2. Dead
MAX_VARIETY_SCORE = 10.0constantPlanningService.javaadds this constant but never references it. Either wire it in or delete it.Suggestions
3.
isChildFriendlyis now a zombie fieldRecipe.javastill has@Column(name = "is_child_friendly", nullable = false)hardcoded tofalseon every create/update. The field is alive in the DB but dead in the application. Either add a migration to drop it, or add a TODO comment explaining it will be replaced by a tag. Right now it's silent dead weight.4.
serves/cookTimeMinsendundefinednotnullfrom the frontendpage.server.tssendsserves ? Number(serves) || undefined : undefined. When openapi-fetch serialises this body,undefinedfields are omitted from JSON. Jackson then defaults the Integer to null (fine now) but it's subtle. Sendingnullexplicitly is clearer intent.5.
SuggestionResponseDTO split is the right callKeeping
SuggestionRecipeslim (no image) whileSlotReciperetains the image is exactly correct. Good separation of concerns.6. Frontend test coverage for heroImageUrl is solid
Both new/edit
page.server.test.tsnow test the image path. TDD followed correctly here.🏛️ Architect — System Design & Layer Boundaries
Verdict: ⚠️ Approved with concerns
Blockers
1.
RecipeSummaryResponseincludesheroImageUrl— list endpoint will become slowStoring images as base64 data URIs in the DB is an acknowledged trade-off, but
RecipeSummaryResponse(the list endpoint) also returnsheroImageUrl. Each recipe in the list will carry potentially 200–500 KB of base64-encoded image data. With 11 seed recipes, the/v1/recipesresponse is already ~2–5 MB. This will become a real performance wall as the collection grows. Consider nulling outheroImageUrlin the summary response or adding a separate thumbnail field.Concerns
2. Out-of-order migration creates a confusing version history
V023__hero_image_text.sqlwas written afterV101__dev_seed_recipes.sqlwas already applied. Flyway logs showoutOfOrder mode is activeand a warning about reproducibility. In a fresh environment this is fine, but in a CI pipeline that applies migrations incrementally this ordering will be visible in the schema history table and can confuse future developers.3.
isChildFriendlyis orphaned at the entity layerRecipe.javahas@Column(name = "is_child_friendly", nullable = false)set permanently tofalse. This is dead state. The DB column persists, Hibernate maps it, but no business logic reads or writes it meaningfully. Either migrate it out (ALTER TABLE recipe DROP COLUMN is_child_friendly) or document the intent to replace it with a tag in a future migration.4.
RecipeCreateRequestis used for both create and updateThis is a common pattern but it means you can never make fields required-on-create-only vs. optional-on-update. Already a tension point:
servesandcookTimeMinare now nullable to accommodate the UI, which weakens the create contract. ConsiderRecipeCreateRequest+RecipeUpdateRequestif this grows.Positives
SuggestionRecipeDTO separation fromSlotRecipeis architecturally correct — responses carry only what the consumer needs.scoreDeltaandhasConflictin the DTO (not computed client-side) is the right boundary decision.🧪 Tester — Test Coverage & Quality
Verdict: 🚫 Changes requested
Blockers
1. No regression test for the
serves=null/cookTimeMin=nullbug that was just fixedRecipeServiceTest.javawas updated to fix constructor calls, but no new test was added that specifically verifies: "whenservesandcookTimeMinare null,createRecipesucceeds and stores 0." The exact bug path (null Integer → short conversion) has no regression coverage.2. No backend test for effort case acceptance
RecipeControllerTest.javatests the happy path with"medium"(lowercase) but there's no explicit test verifying that"Easy"(capitalized, as previously sent by the frontend) returns 400 from the backend. If someone changes the pattern again, there's no safety net.3.
RecipeForm.svelteimage upload has no testRecipeForm.test.tsexists but was not updated to cover: file selection triggeringFileReader, the preview rendering, or the hiddenheroImageUrlinput being populated. The most user-visible new feature has zero component test coverage.Suggestions
4.
e2e/startseite.test.tsappears to be scaffolding onlyThis file was added but likely contains no meaningful assertions. Either add real tests or don't commit the file yet.
5. The heroImageUrl tests in
page.server.test.tsare well-structuredTesting both the provided-image and empty-image cases is exactly right. This is a good example of the TDD pattern being followed correctly.
6. Consider a test for
IngredientEntry.quantity = 0to expose the validation mismatch@DecimalMin("0.01")+ frontend fallback of0= silent 400. A test documenting this behaviour would make the issue visible before it hits production.🔒 Security Expert
Verdict: 🚫 Changes requested
Blockers
1. No size limit on uploaded images — potential DoS / DB exhaustion
heroImageUrlis stored as atextcolumn with no length constraint. The SvelteKit action reads the raw base64 string (formData.get('heroImageUrl') as string) and passes it directly to the backend without any size check. A single request could push megabytes or gigabytes into the DB. Spring Boot's default max request size (1 MB) provides accidental protection, but this should be explicit:Also add a client-side guard in
RecipeForm.sveltebefore theFileReadercall:2. No content-type validation on
heroImageUrlThe backend stores any arbitrary string passed as
heroImageUrl. There is no check that the value starts withdata:image/. Adata:text/html;base64,...ordata:image/svg+xml,...URI could be stored and later rendered. While browsers won't execute scripts from<img src="data:text/html...">, SVG data URIs can run embedded JavaScript when rendered as images in some contexts. Validate that the value matches^data:image/(jpeg|png|webp|gif);base64,before persisting.3. Verify
application-dev.ymlcontains no secretsThis file was committed for the first time in this PR. Ensure it contains no hardcoded DB passwords, JWT secrets, or API keys. If it does, rotate them now — git history persists even after deletion.
Suggestions
4. The form
heroImageUrlhidden field sends the full base64 string as a form fieldThis bypasses normal browser file-upload size limits (which apply to
<input type="file">in multipart forms) and sends the data as a regular form field. The file is already read client-side, but the hidden field approach means no Content-Length header protection at the browser level. This is acceptable for a home app but worth noting.5. No authentication check around image upload
The recipe create/update endpoints are presumably protected (there's an auth layer in the project), so this is likely fine. Just confirm the
/v1/recipesPOST and PUT endpoints require an authenticated session — no anonymous uploads should be possible.🎨 UI/UX Expert
Verdict: ✅ Approved
What works well
Suggestions
1. No feedback while image is being processed
FileReaderis async. For a 4–5 MB image on a slower device, there could be a noticeable delay between file selection and preview appearing with no loading state. A simple spinner or opacity change on the preview area would help.2. No file size feedback to the user
If a large file is selected, the user has no indication it might cause a problem. A small helper text like
"Max. 2 MB"under the upload button sets expectations.3. Upload button has no icon
A
📎or upload arrow icon alongside the text would improve scannability in the form, especially since it sits among text inputs.4.
isChildFriendlyremoval leaves no gapThe field was never visible in the form UI (it was a DTO-only field), so users won't notice. The categories panel now covers all real classification needs. No visible regression.
⚙️ DevOps
Verdict: ⚠️ Approved with concerns
Concerns
1. Spring Boot request size not configured for image uploads
Base64 encoding inflates file size by ~33%. A 1.5 MB JPEG becomes ~2 MB of base64 text. Spring Boot's default
max-request-sizeis 1 MB — this means recipes with images will silently fail to save with a 413 error. Add explicit limits toapplication.yml(orapplication-dev.yml):And the SvelteKit backend also needs to accept larger bodies:
2.
application-dev.ymlis now tracked in gitVerify this file contains no environment-specific secrets. Ideally dev config references env variables (
${DB_PASSWORD}) rather than hardcoded values, so the file is safe to commit.3.
V023out-of-order migrationThe Flyway log on startup shows:
Migrating schema "public" to version "023 - hero image text" [out of order]. This is expected givenoutOfOrder: truein the docker profile, but worth noting: if you ever need to run migrations in CI against a clean DB, V023 will still apply correctly (it just appears after V101 in the history). Not a blocker for this project, but something to be aware of.4.
V101seed data is large (~400 lines of SQL)Startup time on a fresh DB will be slightly longer due to the seed. Not a problem in dev but ensure CI test containers don't inadvertently apply seed migrations (check that the
dev/dockerSpring profile without-of-orderis not active in test containers).LGTM
down -v && up --buildis the right way to force a clean migration run.docker-compose.ymlor CI configuration that would affect deployment.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Good TDD discipline throughout — every behaviour change has a corresponding test, and the red/green/refactor pattern is visible in the commit history. The effort-normalisation fix and null-serves fix are clean. A few things I'd push back on:
Blockers
validateHeroImageUrluses.find()with a^-anchored pattern (RecipeService.java). With a start anchor,.find()and.matches()are functionally equivalent, but this is a subtle footgun — a future author could drop the^and not notice the security regression. Use.matches()explicitly so the intent is obvious:@NotEmptyoningredientscreates a silent 400 (RecipeCreateRequest.java). The frontend filters out any ingredient withquantity <= 0. If a user submits with all-zero quantities, the JSON body contains[], the backend returns 400, but the SvelteKit action returnsfail(500, { error: 'Fehler beim Speichern' })— the user gets no useful feedback. Either: relax@NotEmptyat the backend and let the frontend enforce it, or map 400 responses from the API to a user-facing message.Suggestions
ImageCompressorreads the image twice — once viaImageIO.read()to get the width, then Thumbnailator reads the same bytes again internally. You could passtargetWidthas a parameter and avoid the first decode, or pre-decode once and pass theBufferedImage. Minor perf concern.serves/cookTimeMinnull → 0 inRecipeService.javaloses semantic meaning — 0 portions and "not set" are now indistinguishable in the DB. Consider storingNULLin theshortcolumns or making themIntegerat the entity level.RecipeServiceTest.createRecipeWithAllowedImageTypeShouldNotThrowdoesn't assert the image fields are set on the saved recipe. A stronger test would capture theRecipeargument passed torecipeRepository.save()and verifyheroImageUrlis set.🏛️ Architect — System Design
Verdict: ⚠️ Approved with concerns
The feature works and the layering is correct — validation in the service, compression as a separate
@Component, DTO split between summary (preview) and detail (full URL). That said, there are two structural concerns worth raising before this pattern gets copied.Blockers
max-file-size: 5MB/max-request-size: 6MBinapplication.ymldoes not protect this feature. These Spring limits only apply tomultipart/form-datauploads. The image is submitted as a base64 string inside a JSON body (Content-Type: application/json). The size limits are therefore completely bypassed — a 20MB image in the request body will not be rejected by Spring before hittingRecipeService. The only enforcement is the client-side JS check, which is trivially bypassable. This needs a server-side JSON body size check (either via@SizeonheroImageUrlor a SpringContentCachingRequestWrapperfilter).Suggestions
Base64 images in PostgreSQL
textcolumns are an architectural dead end. A 5MB image becomes ~6.7MB of base64 in the DB. With dozens of recipes this is manageable; with hundreds or household sharing it becomes a problem. For a family app this is acceptable for now, but the migration path to object storage (MinIO/S3) will be painful later. At minimum, add a comment to the entity field noting the intentional trade-off.Compression is synchronous in the request thread (
RecipeService.createRecipe). For a large image this could add hundreds of milliseconds to the HTTP response. For a single-household app this is fine, but worth noting if you ever move to multi-tenant.ValidationException→ HTTP 422 for image type mismatch is a reasonable choice, but 400 Bad Request would be more conventional for an invalid input format. Not a blocker.SuggestionResponse.SuggestionRecipesplit (noheroImagePreview) is the right call — suggestion payloads staying lean is good API design.🔒 Security Expert — Application Security
Verdict: 🚫 Changes requested
There is one actual security gap that needs closing before merge.
Blockers
Server-side image size limit is illusory.
spring.servlet.multipart.max-file-size: 5MBonly gatesmultipart/form-datarequests. This endpoint receivesapplication/jsonwith the image as a base64 string in the body. There is zero server-side size enforcement on the JSON body. A client can send a 50MB base64 string and the server will decode, attempt to compress, and persist it without rejection. Mitigate with one of:@Size(max = 7_000_000)onheroImageUrlinRecipeCreateRequest(base64 of 5MB ≈ 6.7MB, give headroom)Content-Lengthon POST/PUT to/v1/recipesvalidateHeroImageUrlusesPattern.find()instead ofPattern.matches()(RecipeService.java). The pattern has a^anchor so these are equivalent today, but this is a latent correctness risk..matches()is unambiguous and should be used.Observations (not blockers for this app scale)
Client-side file size check is the only real gate. JS can be bypassed via
curlor DevTools. This is acceptable for a private family app but would be a gap in a multi-tenant product.MIME type sniffing is not performed. The backend trusts the data URI prefix (
data:image/jpeg;...) without verifying the actual bytes match. A PNG file renamed with a JPEG header in the data URI will pass validation. Thumbnailator/ImageIO will still decode it correctly, so there's no injection vector, but the content-type claim is unverified.Full-size images are stored in the DB unencrypted. For a self-hosted family app this is fine. If you ever expose this to multiple households, photos of family members sitting in a shared Postgres DB without encryption at rest is worth revisiting.
No rate limiting on the create/update endpoints. An authenticated user could hammer the compress path with large images. Not critical at current scale.
🧪 Tester — Quality & Test Coverage
Verdict: ⚠️ Approved with concerns
Solid test suite overall —
ImageCompressorTestcovers 8 behaviours including edge cases (null, blank, non-data-URI, invalid base64, no-upscale). The page server tests are thorough. A few gaps:Blockers
No test for what happens when
ImageCompressor.compressToPreview()fails at runtime. The method catches allExceptionand returnsnull— so if compression fails (e.g., out of memory, corrupt image that passes base64 decode but failsImageIO.read), the recipe saves withheroImagePreview = nullsilently. There should be a test that mocksimageCompressor.compressToPreview()returningnulland asserts the recipe still saves (if that's the intended behaviour) or throws (if it isn't). Right now the behaviour is unspecified and untested.Suggestions
ImageCompressorTestonly uses PNG input. All 8 tests callmakePngDataUri(). There's no test with a JPEG input, which exercises a different ImageIO codec path. Worth adding onecompressToPreview_acceptsJpegInput()test.No test for the
@NotEmptyingredients boundary. The backend rejects an empty ingredients list with 400, but neither the server action tests nor the controller test exercise this path. A test inRecipeControllerTestsending"ingredients": []would document the contract.createRecipeWithAllowedImageTypeShouldNotThrowinRecipeServiceTestpasses the string"data:image/jpeg;base64,abc"— this is valid base64 but decodes to 2 bytes whichImageIOcannot parse. TheimageCompressormock is not stubbed in this test, so@InjectMocksuses the realImageCompressor, which will silently returnnull. This means the test doesn't actually verify the compression path. If the intent is to test that the service accepts the MIME type, that's fine — but add a comment making the intent explicit.Frontend: no test that submitting with all ingredients having quantity 0 shows a user-friendly error (as opposed to the generic "Fehler beim Speichern"). This is the scenario where
@NotEmptybackend validation fires.🎨 UI/UX Expert — Interface & Experience
Verdict: ⚠️ Approved with concerns
The category grouping with German type headings is a clear improvement over the old flat list. The effort chips and image upload section are functional. A few UX rough edges:
Blockers
No frontend validation for unsupported image types. The backend rejects non-image MIME types (PDF, etc.), but the
<input accept="image/*">attribute only suggests a filter — it's not enforced in all browsers, and it doesn't communicate which image types are accepted. A user uploading a TIFF or HEIC gets a silent generic backend error. ThehandleImageChangefunction should checkfile.typeagainst the allowed types and show a German error message before attempting to read the file.Suggestions
The 5MB error message appears below the upload button — the user's eye is at the label, not below it. When the error fires the user may scroll past it. Consider showing the error inline in a more prominent position, or using the same alert banner as form-level errors.
Bild entfernenis a plain text button with no visual affordance that it's destructive. A user who accidentally clicks it loses the image with no confirmation. Consider either an undo pattern or at minimum a red hover color (you havehover:text-[var(--color-error)]already — this is good — but the default state looks identical to a normal link).No upload progress or loading indicator. For a large file near the 5MB limit,
FileReader.readAsDataURL()is synchronous-ish but can cause a brief UI freeze. A spinner or "Wird geladen…" state on the button would prevent the user from thinking nothing happened.The upload button label switches between "Bild hochladen" and "Bild ändern" but the hidden input with the current image value is always present. This is correct but the toggle is subtle — consider showing a small thumbnail preview (which the code already supports via
{#if heroImageUrl}) more prominently, so the user can confirm the right image was selected before saving.Kochzeitinput has no unit label. The user doesn't know if it's minutes, hours, or something else. Add "(Minuten)" or a small "min" suffix.⚙️ DevOps — Infrastructure & Operations
Verdict: ⚠️ Approved with concerns
Blockers
spring.servlet.multipartsize limits are misleading and should be removed or annotated. (application.yml). The image upload uses a JSON body, not multipart. These limits provide no protection and will mislead future operators into thinking image size is capped at 5MB server-side. Either remove them (they apply to nothing relevant) or add a comment:Suggestions
V025
DROP COLUMN is_child_friendlyis irreversible. If this migration runs in production and the column turns out to be needed, rollback requires a new migration + data recovery. This is acceptable since the column is intentionally removed, but worth noting there's no rollback path.application-dev.ymlis untracked (appears ingit statusas??). If it contains local credentials or overrides, it should either be in.gitignoreor committed as a template (application-dev.yml.example). Make sure it's not accidentally committed in a later push.The seed file
V101__dev_seed_recipes.sqluses version 101 while migrations are at V025. Flyway runs these in order unlessout-of-order: trueis set (whichapplication-dev.ymlpresumably sets). This is fine for dev but means production Flyway config must never apply V101. Verify the production profile explicitly hasflyway.locationspointing only toclasspath:db/migration(notdb/seed).No Docker image size impact assessed. Adding Thumbnailator + TwelveMonkeys imageio-webp adds JARs to the application. Not a concern at this scale but worth tracking in CI if image size becomes a build constraint.
Compression is CPU-bound and synchronous. For a small household app this is fine. If load ever increases, consider a bounded executor for the compress step to avoid blocking the HTTP thread pool.
PR review concerns addressed
All 7 blockers and 6 suggestions from the review have been resolved. Here's a summary by commit:
Blockers fixed
@Size(max=7_000_000)onheroImageUrl)ed769b1.find()→.matches()with.*suffix so full URI is validateded769b1createRecipeWithOversizedHeroImageShouldReturn400ed769b1createRecipeWithEmptyIngredientsListShouldReturn400ed769b1handleImageChange(rejectsimage/bmpand other non-allowlisted types)56e6143fail(422)with explicit message when all ingredients filter to emptyebaf42dcreateRecipeWithCapitalisedEffortShouldReturn400ed769b1Suggestions addressed
RecipeServiceTest: verify null preview stored when compressor returns null9df6d6fImageCompressorTest: add JPEG input test23c8219createRecipeWithAllowedImageTypeShouldNotThrow(why fake base64 is acceptable)b2a798dapplication.yml: annotate multipart limits explaining they don't apply to JSON bodya5bb5d4application-dev.ymlto.gitignore932155c(min)unit hint to Kochzeit label inRecipeForm.svelte73b4fb8Bild entfernenbutton: persistent muted-red color (was hover-only)e5cdce1Test counts after all changes
Note on pre-existing type errors
npm run checkreports 10 type errors in 4 files — all pre-existing before this PR (3 innew/+page.server.tsaroundnullvsundefinedon the API body, and 2 inplanner/variety/+page.sveltefrom theVarietyWarningCardsrefactor). None were introduced by this review session.