fix(recipe): re-read worker-written image metadata; reset generator on route entry [KAN-255] [KAN-256] - #3450
fix(recipe): re-read worker-written image metadata; reset generator on route entry [KAN-255] [KAN-256]#3450adamtasteslikegood wants to merge 6 commits into
Conversation
…n entry [KAN-255] [KAN-256] Two symptoms of one root cause: the recipe lifecycle lives on a root-scoped service, but the DATA half of it was never reconciled with the server. KAN-255 — the image pipeline finishes SERVER-side. The Pub/Sub worker writes `ai_image_gcs`, `ai_metadata.image_generation`, and flips `ai_metadata.image_request.status` / `image_enqueue.status` from `pending` to `complete` (Backend worker_api_bp:650-698). The client wrote back exactly one field, `ai_image_url`, so after nav-away-and-return the in-memory copy and localStorage both still claimed the image was pending — visible the moment a single recipe was exported as JSON. PersistenceService.refreshRecipeFromApi() re-reads the row through the existing column-over-blob merge (KAN-139) and merges it locally. It never POSTs (the row came from the server), never ADDS a row (a cold deep-linked recipe must not be silently saved by a background reconcile), and never rejects (the image already rendered; a failed reconcile is not the user's problem). v0.4.12 (KAN-243) moved the image SPINNER to RecipeStateService so it survives component destruction. This moves the DATA the same way: the reconcile runs after the detached await, gated on `this.recipe()?.id`, so it lands whether or not the component that started it still exists. KAN-256 — `clearRecipe()` fired inside `onGenerate()`, which is submit-time, not entry-time. The generated recipe outlives the component, so navigating back to the generator re-rendered the previous recipe under an empty prompt. Moved to the constructor, which is the route-entry hook: navigating to `/` recreates the component, staying on `/` reuses it. In-flight image generation is untouched — it is tracked by recipe id on the service, so the recipe's own page keeps its spinner and the reconcile above still lands. Consolidated `GeneratorComponent.triggerImageGeneration` and `RecipeViewBase.regenerateImage` into one `runImageGeneration()`. They differed only in the force flag and one toast string; keeping two copies would have meant applying the reconcile twice, which is how one of them ends up without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BjAKLhqnWqpXeJ6z1uH3JW
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Address comments before merging to prevent image display blanking and undefined field overwrites during metadata reconciliation. 📝 Suggestions are shown in the inline comments below. |
🔍 Review Summary📋 Acceptance criteriaThe issue is ready for review, and the below acceptance criteria have been met:
Code Reviewer could not determine whether the following acceptance criteria have been met:
|
🔍 Review Summary📋 Acceptance criteriaThe issue is ready for review. Code Reviewer could not determine whether the following acceptance criteria have been met:
|
adamtasteslikegood
left a comment
There was a problem hiding this comment.
Review after synchronization. The unified image-generation path correctly re-reads worker-authored metadata, updates saved local state without adding cold deep links, preserves canonical URLs while keeping cache-busters display-only, and guards view updates across navigation. Route-entry generator reset is appropriately scoped. Completed exact-head build/security/format checks pass; refreshed PR and independent reviews are pending. No blocking findings remain.
Independent code review — summaryReviewed the diff ( Two advisory findings, both posted inline. No fixes auto-applied — see per-finding rationale on each thread.
Everything else in the diff looks sound: the two-copy consolidation into Advisory only — do not block on this review. — Independent Claude review (claude-opus-4-7, effort medium) |
🔍 Review Summary📋 Acceptance criteriaThe issue is ready for review. Code Reviewer could not determine whether the following acceptance criteria have been met:
|
🔍 Review Summary📋 Acceptance criteriaThe issue is ready for review. Code Reviewer could not determine whether the following acceptance criteria have been met:
|
🔍 Review Summary📋 Acceptance criteriaThe issue is ready for review. Code Reviewer could not determine whether the following acceptance criteria have been met:
|
There was a problem hiding this comment.
Independent CI review — three findings on the KAN-255 reconcile, all PLAUSIBLE (worth surfacing, none merge-blocking). Full context on each is inline; short version below.
- Reconcile can clobber concurrent edits.
syncImageMetadata's wholesalethis.recipe.set(fresh)— plusrefreshRecipeFromApi's innerauth.saveRecipe(fresh)— has a wider race window than a normal write-race because the image gen keeps the reconcile pending for tens of seconds. AsaveNotesortogglePublicinitiated in that window whose POST hasn't landed yet gets overwritten when the reconcile's GET returns the pre-edit row. The docstring calls the wholesale-adopt deliberate; the width of the window is what makes this worth flagging. refreshRecipeFromApihas no direct unit tests. All coverage is via a wholesale mock inrecipe-view.base.test.ts. Its four guards (auth check, non-OK response, id-mismatch, cold-deep-link-vs-saved membership) would regress silently.interpretSaveResponsewas extracted next door for exactly this reason.- Reconcile tests skip
recipeFromRow. The mock returnsSERVER_ROW.data(bare blob), so the tests never exercise the column-over-blob merge that actually runs in production. This is also what makes theis_public/slugcolumn-overwrite invisible to the suite.
No code changes applied — finding 1 is a design conversation (wholesale-adopt is documented as deliberate) and 2/3 are additive test work I'd rather leave to the author's judgment on assertions.
— Independent Claude review (claude-opus-4-7, effort high)
Replied by Claude on Adam's behalf
…55] [KAN-256] Review findings on #3450. The KAN-255 reconcile replaced the viewed recipe and the localStorage row wholesale with the row returned by GET /api/recipes/:id. That GET fires when an image settles, 30-60s after the user asked for it — wide enough for the user to edit personal notes or hit publish, and for that write's POST to reach the server AFTER the reconcile's GET has read the pre-edit row. The wholesale set/save then reverted the edit on screen and in localStorage. The localStorage half does not self-heal: saveNotes POSTs the whole recipe, so the next save after a clobber writes the stale copy back to the server and the edit is lost for good. Pre-PR the image path only did a targeted updateRecipeField('ai_image_url', ...), so this was a regression introduced here rather than a pre-existing tradeoff. Adds adoptImagePipelineFields(local, fresh) in utils/recipe-row.ts, adopting exactly the fields the reconcile exists to read back — ai_image_url, ai_image_gcs, ai_metadata — and keeping every other field local. The client never writes ai_metadata (only gemini.service reads it), so those are worker-owned. A field absent from the server row leaves the local value alone rather than clearing it, so a row without ai_image_url cannot blank the image on screen. Applied at both layers: the view signal merges onto what is displayed, persistence merges onto the saved row. syncImageMetadata is the only caller of refreshRecipeFromApi, so no other contract depended on the wholesale adopt. Tests: direct coverage for the pure helper in recipe-row.test.ts (the interpretSaveResponse precedent — the service constructor registers an effect, so the pure function is the testable seam), plus a view-level test that a notes edit made during generation survives. The reconcile mocks now run the real recipeFromRow(SERVER_ROW) merge instead of handing back the bare blob, so the column-over-blob overlay is exercised. Both new behaviours were confirmed to fail against the wholesale version before the fix was restored. Also already present at f5d8cb3 and verified: the recipeId in the warn log and the fresh.id !== recipeId guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BvEu7ANRWdhTQVsLXhHgWj
| // Adopt the pipeline's fields onto what is on screen rather than replacing | ||
| // it. The user may have edited notes or hit publish while the image was | ||
| // generating, and the row this GET read predates that write. | ||
| this.recipe.set(adoptImagePipelineFields(current, fresh)); | ||
| // Rebuild rather than copy: `fresh.ai_image_url` is canonical (it must | ||
| // stay that way — see above), and the `_t` display marker lives on the | ||
| // service, keyed by id. | ||
| this.generatedImageUrl.set(this.recipeState.imageDisplayUrl(recipeId, fresh.ai_image_url)); |
There was a problem hiding this comment.
| // Adopt the pipeline's fields onto what is on screen rather than replacing | |
| // it. The user may have edited notes or hit publish while the image was | |
| // generating, and the row this GET read predates that write. | |
| this.recipe.set(adoptImagePipelineFields(current, fresh)); | |
| // Rebuild rather than copy: `fresh.ai_image_url` is canonical (it must | |
| // stay that way — see above), and the `_t` display marker lives on the | |
| // service, keyed by id. | |
| this.generatedImageUrl.set(this.recipeState.imageDisplayUrl(recipeId, fresh.ai_image_url)); | |
| // Adopt the pipeline's fields onto what is on screen rather than replacing | |
| // it. The user may have edited notes or hit publish while the image was | |
| // generating, and the row this GET read predates that write. | |
| const merged = adoptImagePipelineFields(current, fresh); | |
| this.recipe.set(merged); | |
| // Rebuild rather than copy: `merged.ai_image_url` is canonical (it must | |
| // stay that way — see above), and the `_t` display marker lives on the | |
| // service, keyed by id. | |
| this.generatedImageUrl.set(this.recipeState.imageDisplayUrl(recipeId, merged.ai_image_url)); |
Use the merged recipe's image URL so that if the server response omits the field, the display does not blank.
| export function adoptImagePipelineFields(local: Recipe, fresh: Recipe): Recipe { | ||
| const merged = { ...local } as Record<string, unknown>; | ||
| const source = fresh as unknown as Record<string, unknown>; | ||
| for (const field of IMAGE_PIPELINE_FIELDS) { | ||
| if (field in source) merged[field] = source[field]; | ||
| } | ||
| return merged as unknown as Recipe; | ||
| } |
There was a problem hiding this comment.
| export function adoptImagePipelineFields(local: Recipe, fresh: Recipe): Recipe { | |
| const merged = { ...local } as Record<string, unknown>; | |
| const source = fresh as unknown as Record<string, unknown>; | |
| for (const field of IMAGE_PIPELINE_FIELDS) { | |
| if (field in source) merged[field] = source[field]; | |
| } | |
| return merged as unknown as Recipe; | |
| } | |
| export function adoptImagePipelineFields(local: Recipe, fresh: Recipe): Recipe { | |
| const merged = { ...local } as Record<string, unknown>; | |
| const source = fresh as unknown as Record<string, unknown>; | |
| for (const field of IMAGE_PIPELINE_FIELDS) { | |
| if (source[field] !== undefined) merged[field] = source[field]; | |
| } | |
| return merged as unknown as Recipe; | |
| } |
Check that the source field is not undefined to avoid overwriting existing local fields when the property value is undefined.
What
Two symptoms, one root cause. v0.4.12 (KAN-243) moved the image-generation spinner onto
RecipeStateServiceso it survives component destruction. It did not move the data.KAN-255 — image metadata never re-read after nav-away
The image pipeline finishes server-side. The Pub/Sub worker writes
ai_image_gcs,ai_metadata.image_generation, and flipsai_metadata.image_request.status/ai_metadata.image_enqueue.statusfrompendingtocomplete(Backend/blueprints/worker_api_bp.py:649-698,_image_generation_metadataat :233-263).The client wrote back exactly one field:
So after generating a recipe, navigating away while the image renders, and coming back, the client's copy still read:
ai_metadata.image_request.status"pending""complete"ai_metadata.image_enqueue.status"pending""complete"ai_metadata.image_generation{ success: true, user_display_name: "Background Worker", ... }ai_image_gcsgs://<bucket>/<id>/<claim-token>.pngexportRecipe()stringifies the viewed recipe verbatim, so a single-recipe JSON export showed all of it.Fix:
PersistenceService.refreshRecipeFromApi(id)re-reads the row through the existing column-over-blob merge (recipeFromRow, KAN-139) and merges it into local state after the image settles. Deliberately narrow: never POSTs (the row came from the server), never adds a row (a cold deep-linked recipe must not be silently saved into the cookbook by a background reconcile), never rejects (the image already rendered; a failed reconcile is not the user's problem).Because it runs after the detached
awaitand every write to the viewed recipe is gated onthis.recipe()?.id === recipeId, it lands whether or not the component that started it still exists — the same property KAN-243 gave the spinner.KAN-256 — generator did not reset on route entry
clearRecipe()fired insideonGenerate()— submit-time, not entry-time. The recipe lives on a root-scoped singleton, so navigating back to/re-rendered the previous result under an empty prompt box. Moved to the constructor: route entry destroys and recreates the component, while staying on/reuses the instance (so a result you are still reading is not wiped).It deliberately does not cancel in-flight image generation — that is tracked by recipe id on the service, so the recipe's own page keeps its spinner and the KAN-255 reconcile still lands.
DRY
GeneratorComponent.triggerImageGenerationandRecipeViewBase.regenerateImagewere near-identical, differing only in the force flag and one toast string. Consolidated into oneRecipeViewBase.runImageGeneration(). Keeping two copies would have meant applying the reconcile twice — which is exactly how one copy ends up without it.Testing
recipe-view.base.test.tscovering the reconcile: adopts the worker-written fields, exported JSON deep-equals the API row, unreadable row leaves the optimistic write standing, no clobber when the user navigated to a different recipe, reconcile still runs after nav-away, no reconcile on failure.generator.component.test.tsfor the route-entry reset, including that it does not discard in-flight image tracking.npm run lint,format:check,type-check,npm testall green (458/459; the singleredirects.test.ts/favicon.icofailure is the known dot-path environment artifact, reproduces on a cleandevin this worktree).Not verified here
The live end-to-end repro (generate → nav away → return → export) needs a running Flask + Pub/Sub worker. The field list above is derived from the Backend source, not observed at runtime — flagged rather than claimed.
Scope
src/only. No Backend change, noserver/change. KAN-195 (SSR image cache) is a separate PR against the Backend and touches a disjoint layer (SSR display URL, not client data).Jira: KAN-255, KAN-256
🤖 Generated with Claude Code
https://claude.ai/code/session_01BjAKLhqnWqpXeJ6z1uH3JW
Rovo Dev code review: Out of Rovo Dev credits
You've used all your Rovo Dev credits, so Rovo Dev can't review your pull requests.