feat(persistence): generation persistence — client snapshot + durable media bytes - #999
Open
AlemTuzlak wants to merge 13 commits into
Open
feat(persistence): generation persistence — client snapshot + durable media bytes#999AlemTuzlak wants to merge 13 commits into
AlemTuzlak wants to merge 13 commits into
Conversation
Layer a lightweight, read-only resume snapshot onto media generation. As a run streams, the client builds a GenerationResumeSnapshot (run identity, status, errors, result metadata + artifact refs — never media bytes) and writes it to an optional GenerationServerPersistence store. - ai-client: GenerationResumeSnapshot types + updateGenerationResumeSnapshot reducer; GenerationClient/VideoGenerationClient observe chunks, persist snapshots (serialized queue, warn-not-throw), expose getResumeSnapshot(); disposed guard. No resume() action (stream re-attach is PR #955). - ai-event-client: optional threadId/runId on generation events. - react/solid/vue/svelte/angular hooks: persistence + initialResumeSnapshot options; expose resumeSnapshot/resumeState (+ pending/result artifacts). - example: Persisted mode on the image generation route. - docs: persistence/generation-persistence.md + nav entry. Pairs with the existing withGenerationPersistence server middleware.
Drop the bespoke `GenerationServerPersistence` type and the `{ server }`
option wrapper. The `persistence` option is now a bare storage adapter
reusing the shared `ChatStorageAdapter` contract (aliased as
`GenerationPersistence`), so `localStoragePersistence` /
`sessionStoragePersistence` / `indexedDBPersistence` work for generations
exactly as they do for chat — matching main's ergonomics.
…istence (no call-site generic)
Default `localStoragePersistence` / `sessionStoragePersistence` /
`indexedDBPersistence` to a value-agnostic `TValue` so a bare, unannotated
call works for BOTH chat and generation persistence — the consuming
`persistence` option constrains the stored value. Generation docs/example now
use `localStoragePersistence({ keyPrefix })` with no type declaration.
PR #955 (resumable streams) is merged, so delivery durability is available today — it was wrongly described as an unlanded future feature. Rewrite the generation-persistence doc: the server example now wires a durability adapter + GET handler, and the delivery section explains that a dropped mid-generation connection re-attaches through the same adapters useChat uses. Clarify that the read-only snapshot carries run state (incl. runId) across reloads, while hooks do not auto-resume on mount.
Server-side artifact + blob storage for generated media, layered on withGenerationPersistence. - @tanstack/ai: result-transform machinery (resultTransforms/artifactInputs on GenerationMiddlewareContext, applyGenerationResultTransforms), threadId/ runId on the image/audio/speech/transcription activities, and generation:artifacts emission from streamGenerationResult. - @tanstack/ai-utils: base64ToUint8Array. - @tanstack/ai-persistence: ArtifactStore + BlobStore contracts, in-memory impls in memoryPersistence(), and withGenerationPersistence byte-persistence (writes bytes to blobs, records ArtifactRecord, attaches PersistedArtifactRef, emits generation:artifacts) with extractArtifacts/nameArtifact options.
…+ blobs, serve route, extractArtifacts/nameArtifact)
Add retrieveArtifact(persistence, id) and retrieveBlob(persistence, idOrRecord) so a serve handler fetches a persisted generation artifact's metadata and bytes without hand-rolling the blob key. artifactBlobKey is the shared key builder used by both withGenerationPersistence (write) and retrieveBlob (read).
The base removed the @tanstack/ai-persistence-drizzle/sqlite subpath, so the state-persistence example now uses memoryPersistence() (with a note to back it with a durable backend in production), matching the byte-storage example.
Contributor
📝 WalkthroughWalkthroughGeneration persistence adds artifact/blob storage middleware, resumable generation snapshots, lifecycle-safe client behavior, framework API exposure, activity result transforms, retrieval helpers, documentation, examples, and tests across the AI packages. ChangesGeneration persistence
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GenerationClient
participant GenerationMiddleware
participant ArtifactStore
participant BlobStore
Client->>GenerationClient: start generation
GenerationClient->>GenerationMiddleware: process generation stream
GenerationMiddleware->>BlobStore: store generated bytes
GenerationMiddleware->>ArtifactStore: store artifact metadata
GenerationMiddleware-->>GenerationClient: artifact references
GenerationClient-->>Client: resume snapshot and result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
🚀 Changeset Version Preview17 package(s) bumped directly, 34 bumped as dependents. 🟥 Major bumps
🟨 Minor bumps
🟩 Patch bumps
|
|
View your CI Pipeline Execution ↗ for commit 5ec5ad9
☁️ Nx Cloud last updated this comment at |
@tanstack/ai
@tanstack/ai-acp
@tanstack/ai-angular
@tanstack/ai-anthropic
@tanstack/ai-bedrock
@tanstack/ai-claude-code
@tanstack/ai-client
@tanstack/ai-code-mode
@tanstack/ai-code-mode-skills
@tanstack/ai-codex
@tanstack/ai-devtools-core
@tanstack/ai-durable-stream
@tanstack/ai-elevenlabs
@tanstack/ai-event-client
@tanstack/ai-fal
@tanstack/ai-gemini
@tanstack/ai-grok
@tanstack/ai-grok-build
@tanstack/ai-groq
@tanstack/ai-isolate-cloudflare
@tanstack/ai-isolate-node
@tanstack/ai-isolate-quickjs
@tanstack/ai-mcp
@tanstack/ai-memory
@tanstack/ai-mistral
@tanstack/ai-ollama
@tanstack/ai-openai
@tanstack/ai-opencode
@tanstack/ai-openrouter
@tanstack/ai-persistence
@tanstack/ai-preact
@tanstack/ai-react
@tanstack/ai-react-ui
@tanstack/ai-sandbox
@tanstack/ai-sandbox-cloudflare
@tanstack/ai-sandbox-daytona
@tanstack/ai-sandbox-docker
@tanstack/ai-sandbox-local-process
@tanstack/ai-sandbox-sprites
@tanstack/ai-sandbox-vercel
@tanstack/ai-solid
@tanstack/ai-solid-ui
@tanstack/ai-svelte
@tanstack/ai-utils
@tanstack/ai-vue
@tanstack/ai-vue-ui
@tanstack/openai-base
@tanstack/preact-ai-devtools
@tanstack/react-ai-devtools
@tanstack/solid-ai-devtools
commit: |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (11)
packages/ai-persistence/tests/generation-artifacts.test.ts (1)
28-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
void (undefined as unknown as T)statements.These exist only to mark the type imports as used and emit real (if inert) runtime statements. Either exercise the types in an actual assertion or rely on
import typebeing erased.♻️ Suggested cleanup
-void (undefined as unknown as GenerationArtifactDescriptor) -void (undefined as unknown as GenerationArtifactExtractionInput) -void (undefined as unknown as GenerationArtifactNameInput) -🤖 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/ai-persistence/tests/generation-artifacts.test.ts` around lines 28 - 30, Remove the inert void-cast statements for GenerationArtifactDescriptor, GenerationArtifactExtractionInput, and GenerationArtifactNameInput. Use type-only imports for these symbols, or reference them through a meaningful type assertion if the test requires type coverage, without emitting runtime statements..changeset/generation-persistence.md (1)
14-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant duplicate intro sentences in the changeset body.
Line 14 ("Add generation persistence: a lightweight client resume snapshot plus optional durable storage of the generated media bytes.") and line 18 ("Add client-side generation persistence: a lightweight, read-only resume snapshot for media generation activities.") restate the same thing. This reads like two changesets were concatenated without a transition, and will show up verbatim in the CHANGELOG.
Consider merging into a single intro followed by the "Media byte storage" and client-snapshot sections as subheadings.
🤖 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 @.changeset/generation-persistence.md around lines 14 - 22, Remove the redundant introductory sentence from the changeset body and keep one concise introduction covering client resume snapshots and optional durable media storage. Preserve the existing “Media byte storage” and client-side generation persistence sections, using them as the detailed follow-up content.packages/ai-solid/src/use-generate-audio.ts (1)
59-72: 📐 Maintainability & Code Quality | 🔵 TrivialRedundant re-declaration of inherited members.
UseGenerateAudioReturnalready inheritsresult,isLoading,error, andstatusunchanged fromOmit<UseGenerationReturn<TOutput>, 'generate'>— onlygenerateneeded restating (its input type differs). The extra re-declarations are dead weight that could silently drift from the base type in the future.♻️ Suggested cleanup
export interface UseGenerateAudioReturn< TOutput = AudioGenerationResult, > extends Omit<UseGenerationReturn<TOutput>, 'generate'> { /** Trigger audio generation */ generate: (input: AudioGenerateInput) => Promise<void> - /** The generation result containing audio, or null */ - result: Accessor<TOutput | null> - /** Whether generation is in progress */ - isLoading: Accessor<boolean> - /** Current error, if any */ - error: Accessor<Error | undefined> - /** Current state of the generation */ - status: Accessor<GenerationClientState> }🤖 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/ai-solid/src/use-generate-audio.ts` around lines 59 - 72, Remove the redundant result, isLoading, error, and status members from UseGenerateAudioReturn, retaining only the generate override whose input type differs. Continue inheriting the unchanged members through Omit<UseGenerationReturn<TOutput>, 'generate'>.packages/ai-client/src/generation-types.ts (1)
273-289: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
ARTIFACTSchunks replace, rather than accumulate,pendingArtifacts.If the server emits artifact refs in more than one
generation:artifactsevent for a run, only the last batch survives. If batching is intentional (one event per run), a short comment stating that would prevent future confusion; otherwise merge byartifactId.🤖 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/ai-client/src/generation-types.ts` around lines 273 - 289, The CUSTOM chunk handling for GENERATION_EVENTS.ARTIFACTS currently overwrites pendingArtifacts, losing earlier batches. Update this branch to merge new artifact references with the existing next.pendingArtifacts by artifactId while preserving prior entries; if the single-event behavior is intentional instead, document that contract with a concise comment.packages/ai-client/tests/generation-resume-state.test.ts (1)
180-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test passes vacuously.
createGenerationResultSnapshotnever reads a top-levelurlfield, soexpect(snapshot.result).not.toHaveProperty('url')holds for any input — including a short, durablehttps:URL. If the intent is to pin the durable-URL policy, exercisedurableUrlFielddirectly through an artifact ref'sexternalUrl(data:, blob:, and >2048 chars) and assert the valid https case is retained.🤖 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/ai-client/tests/generation-resume-state.test.ts` around lines 180 - 214, The test around createGenerationResultSnapshot currently passes without validating URL filtering because it supplies a top-level url that the snapshot builder ignores. Update it to exercise durableUrlField through an artifact ref’s externalUrl, covering data:, blob:, and URLs over 2048 characters as omitted while confirming a valid https URL is retained.packages/ai-angular/src/inject-generation.ts (1)
113-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
resumeState/pendingArtifacts/resultArtifactsinstead of manually syncing 4 signals.
resumeState,pendingArtifacts, andresultArtifactsare separate mutable signals thatsetResumeSnapshotStatemust remember to update in lockstep withresumeSnapshoton every change. Solid's equivalentuseGenerationavoids this duplication entirely by deriving these three fields as simple closures over a singleresumeSnapshotsignal (resumeState: () => resumeSnapshot()?.resumeState ?? null, etc.), with no manual sync path to drift. Angular'scomputed()gives the same guarantee here.♻️ Proposed refactor using `computed()`
- const resumeState = signal<GenerationResumeState | null>( - options.initialResumeSnapshot?.resumeState ?? null, - ) - const pendingArtifacts = signal<Array<GenerationPendingArtifact>>( - options.initialResumeSnapshot?.pendingArtifacts ?? [], - ) - const resultArtifacts = signal<Array<PersistedArtifactRef>>( - options.initialResumeSnapshot?.result?.artifacts ?? [], - ) + const resumeState = computed( + () => resumeSnapshot()?.resumeState ?? null, + ) + const pendingArtifacts = computed( + () => resumeSnapshot()?.pendingArtifacts ?? [], + ) + const resultArtifacts = computed( + () => resumeSnapshot()?.result?.artifacts ?? [], + ) let disposed = false const setResumeSnapshotState = ( snapshot: GenerationResumeSnapshot | undefined, ) => { if (disposed) return resumeSnapshot.set(snapshot) - resumeState.set(snapshot?.resumeState ?? null) - pendingArtifacts.set(snapshot?.pendingArtifacts ?? []) - resultArtifacts.set(snapshot?.result?.artifacts ?? []) }and in the return statement (
computed()results are already read-only, so drop.asReadonly()on these three):resumeSnapshot: resumeSnapshot.asReadonly(), - resumeState: resumeState.asReadonly(), - pendingArtifacts: pendingArtifacts.asReadonly(), - resultArtifacts: resultArtifacts.asReadonly(), + resumeState, + pendingArtifacts, + resultArtifacts,Please confirm
computedis imported from@angular/corein this file (not shown in the unshown import lines) before applying.Also applies to: 240-243
🤖 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/ai-angular/src/inject-generation.ts` around lines 113 - 139, Replace the mutable resumeState, pendingArtifacts, and resultArtifacts signals in the generation setup with Angular computed values derived from the single resumeSnapshot signal, confirming computed is imported from `@angular/core`. Remove their manual updates from setResumeSnapshotState, and return the computed values directly without asReadonly() while preserving the existing fallback values.packages/ai-solid/src/use-generation.ts (1)
223-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGarbled em dash in comment.
Same mojibake (
—) as inuse-generate-video.ts; see consolidated comment.🤖 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/ai-solid/src/use-generation.ts` around lines 223 - 224, Replace the garbled em dash in the mount-devtools comment near the generation flow with the correct dash character, matching the corresponding comment in use-generate-video.ts.packages/ai-solid/src/use-generate-video.ts (1)
256-257: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGarbled em dash in comment.
—is a mojibake artifact for—. Same pattern recurs inuse-generation.ts; see consolidated comment.🤖 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/ai-solid/src/use-generate-video.ts` around lines 256 - 257, Replace the garbled “—” sequence in the mount/devtools comment with the intended em dash “—”, and apply the same comment-text correction in the corresponding occurrence in use-generation.ts.packages/ai-react/src/use-transcription.ts (1)
33-36: 📐 Maintainability & Code Quality | 🔵 TrivialSame duplication concern as the other react generation hooks.
See consolidated comment; this hook duplicates
persistence/initialResumeSnapshot/resume-return fields instead of extending sharedUseGenerationOptions/UseGenerationReturnlike the Solid equivalent.Also applies to: 73-80
🤖 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/ai-react/src/use-transcription.ts` around lines 33 - 36, Update the transcription hook’s options and return types to extend the shared UseGenerationOptions and UseGenerationReturn types, removing the duplicated persistence, initialResumeSnapshot, and resume-related fields. Follow the existing Solid generation hook pattern while preserving the hook’s transcription-specific fields and behavior.packages/ai-react/src/use-generate-speech.ts (1)
33-36: 📐 Maintainability & Code Quality | 🔵 TrivialSame duplication concern as
use-generate-image.ts: options/return fields aren't inherited from a shared type.See consolidated comment for details; the Solid equivalent hook uses
Pick/OmitonUseGenerationOptions/UseGenerationReturnto avoid this duplication.Also applies to: 73-80
🤖 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/ai-react/src/use-generate-speech.ts` around lines 33 - 36, Update the options and return types in the speech generation hook to derive shared fields from UseGenerationOptions and UseGenerationReturn using Pick/Omit, matching the pattern in the Solid equivalent and use-generate-image.ts. Keep only speech-specific additions such as persistence and initialResumeSnapshot locally defined, and remove duplicated inherited field declarations.packages/ai-react/src/use-generate-image.ts (1)
33-36: 📐 Maintainability & Code Quality | 🔵 TrivialConsider extending shared
UseGenerationOptions/UseGenerationReturnlike the Solid hooks now do.The Solid counterpart (
packages/ai-solid/src/use-generate-image.ts) was refactored in this same PR to inheritpersistence/initialResumeSnapshotand the resume/artifact return fields viaPick/Omiton the shared generation types, instead of duplicating them. This React hook (and its siblinguse-generate-speech.ts/use-transcription.ts) still hand-duplicates the same fields and JSDoc, risking drift as the shared contract evolves.Also applies to: 73-88
🤖 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/ai-react/src/use-generate-image.ts` around lines 33 - 36, Refactor the React generation hooks to derive persistence, initialResumeSnapshot, and resume/artifact return fields from the shared UseGenerationOptions and UseGenerationReturn types, following the Pick/Omit pattern used by the Solid useGenerateImage hook. Remove the duplicated declarations and JSDoc from useGenerateImage, useGenerateSpeech, and useTranscription while preserving their existing public behavior.
🤖 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 `@docs/persistence/generation-persistence.md`:
- Line 237: Update the sentence beginning “A full page reload” to hyphenate the
compound adjective as “A full-page reload,” leaving the rest of the sentence
unchanged.
- Around line 147-165: The documented artifact GET example reads and serves
bytes without verifying caller authorization. Update the GET handler before
retrieveArtifact to derive the caller identity from trusted server-side session
state and authorize access to artifactId against the owning user or thread; only
perform retrieveArtifact and retrieveBlob after authorization, and clearly mark
this check as required for production routes.
In `@packages/ai-angular/src/inject-generation.ts`:
- Around line 217-218: Replace the mojibake em-dash in the comment near the
devtools-only generation logic in packages/ai-angular/src/inject-generation.ts
(lines 217-218) with a proper em dash; also replace both mojibake occurrences in
the useGenerateAudio transport-mode JSDoc in
packages/ai-react/src/use-generate-audio.ts (lines 83-88), without changing the
surrounding wording.
In `@packages/ai-client/src/generation-client.ts`:
- Around line 176-179: Move the disposed guard in generate ahead of
mountDevtools so calls after disposal return without re-registering devtools or
emitting bridge events. Preserve the existing isLoading guard and normal
devtools mounting for active clients.
- Around line 537-548: Update completePlainFetcherResumeSnapshot() to create a
new completed resume snapshot when this.resumeSnapshot is undefined, using the
client’s established snapshot shape and current generation context. Preserve
existing seeded-snapshot updates, callback notification, and persistence
behavior, and inspect VideoGenerationClient’s analogous fetcher completion hook
for the same missing initialization.
In `@packages/ai-client/src/video-generation-client.ts`:
- Around line 614-621: Update observeResumeSnapshot and the related persistence
flow so frequent stream chunks do not enqueue one storage write each. Coalesce
or debounce pending persistResumeSnapshot work to retain only the latest resume
snapshot, while preserving callback updates; ensure terminal events still
persist the final snapshot unconditionally.
- Around line 651-664: Update writeResumeSnapshot so a successful
serverPersistence.setItem call clears resumePersistenceError before returning.
Keep the existing error assignment and warning behavior in the catch block
unchanged, so getResumePersistenceError reflects only the latest write failure.
- Around line 623-634: Update completePlainFetcherResumeSnapshot so
plain-fetcher mode creates a terminal resume snapshot when this.resumeSnapshot
is undefined, using the configured persistence-compatible snapshot shape and
status "complete" with resumeState null. Preserve existing snapshot fields and
callback/persistence behavior when a snapshot already exists, ensuring fetcher
mode with persistence always persists the terminal state.
In `@packages/ai-event-client/src/index.ts`:
- Around line 615-618: Retain threadId and runId in the event interfaces in
packages/ai-event-client/src/index.ts, and update every lifecycle event payload
in generateAudio, generateImage, generateSpeech, and generateTranscription to
populate both IDs consistently across request, usage, completion, and error
emitters. Ensure each producer-side event includes the correlation values
available for its streamed or persisted run.
In `@packages/ai-persistence/src/middleware.ts`:
- Around line 46-53: Resolve the duplicate WithPersistenceOptions declarations
by keeping chat persistence options separate from generation persistence
options: rename the generation-specific declaration and update
withGenerationPersistence to use it, or consolidate only if both APIs
intentionally share all options. Ensure withPersistence and
withGenerationPersistence no longer accidentally accept each other’s option
fields.
- Around line 332-345: Update parseDataUrl to parse the data URL’s media type
and semicolon-delimited parameters generically, allowing parameters such as
codecs or charset before an optional final base64 marker. Return the MIME type
without parameters, decode the payload according to whether base64 is present,
and preserve undefined for invalid data URLs so callers do not process the
original data: string.
- Around line 617-655: Harden URL handling in the descriptor.url fetch path:
validate and allow only http/https URLs, invoke the existing allowlist or SSRF
guard hook before fetch, and use an AbortSignal timeout. Enforce a maximum
response size for both streamed and buffered bodies, aborting or rejecting
oversized responses before persisting them; preserve data-URL handling and
metadata behavior.
In `@packages/ai-persistence/src/retrieve.ts`:
- Around line 14-45: Update the JSDoc for retrieveArtifact and retrieveBlob to
state that artifact identifiers are opaque lookup keys and callers must derive
identity server-side and verify record.threadId ownership at the route boundary
before serving bytes. Remove or qualify wording that suggests mapping a null
result directly to a 404 without authorization.
In `@packages/ai-vue/src/use-generation.ts`:
- Around line 187-198: Guard the public onResult callbacks against disposed
scopes, matching the existing onError/onProgress/onChunk behavior. In
packages/ai-vue/src/use-generation.ts lines 187-198, return without invoking
options.onResult when disposed; apply the identical change to the baseOptions
onResult callback in packages/ai-vue/src/use-generate-video.ts lines 203-214.
---
Nitpick comments:
In @.changeset/generation-persistence.md:
- Around line 14-22: Remove the redundant introductory sentence from the
changeset body and keep one concise introduction covering client resume
snapshots and optional durable media storage. Preserve the existing “Media byte
storage” and client-side generation persistence sections, using them as the
detailed follow-up content.
In `@packages/ai-angular/src/inject-generation.ts`:
- Around line 113-139: Replace the mutable resumeState, pendingArtifacts, and
resultArtifacts signals in the generation setup with Angular computed values
derived from the single resumeSnapshot signal, confirming computed is imported
from `@angular/core`. Remove their manual updates from setResumeSnapshotState, and
return the computed values directly without asReadonly() while preserving the
existing fallback values.
In `@packages/ai-client/src/generation-types.ts`:
- Around line 273-289: The CUSTOM chunk handling for GENERATION_EVENTS.ARTIFACTS
currently overwrites pendingArtifacts, losing earlier batches. Update this
branch to merge new artifact references with the existing next.pendingArtifacts
by artifactId while preserving prior entries; if the single-event behavior is
intentional instead, document that contract with a concise comment.
In `@packages/ai-client/tests/generation-resume-state.test.ts`:
- Around line 180-214: The test around createGenerationResultSnapshot currently
passes without validating URL filtering because it supplies a top-level url that
the snapshot builder ignores. Update it to exercise durableUrlField through an
artifact ref’s externalUrl, covering data:, blob:, and URLs over 2048 characters
as omitted while confirming a valid https URL is retained.
In `@packages/ai-persistence/tests/generation-artifacts.test.ts`:
- Around line 28-30: Remove the inert void-cast statements for
GenerationArtifactDescriptor, GenerationArtifactExtractionInput, and
GenerationArtifactNameInput. Use type-only imports for these symbols, or
reference them through a meaningful type assertion if the test requires type
coverage, without emitting runtime statements.
In `@packages/ai-react/src/use-generate-image.ts`:
- Around line 33-36: Refactor the React generation hooks to derive persistence,
initialResumeSnapshot, and resume/artifact return fields from the shared
UseGenerationOptions and UseGenerationReturn types, following the Pick/Omit
pattern used by the Solid useGenerateImage hook. Remove the duplicated
declarations and JSDoc from useGenerateImage, useGenerateSpeech, and
useTranscription while preserving their existing public behavior.
In `@packages/ai-react/src/use-generate-speech.ts`:
- Around line 33-36: Update the options and return types in the speech
generation hook to derive shared fields from UseGenerationOptions and
UseGenerationReturn using Pick/Omit, matching the pattern in the Solid
equivalent and use-generate-image.ts. Keep only speech-specific additions such
as persistence and initialResumeSnapshot locally defined, and remove duplicated
inherited field declarations.
In `@packages/ai-react/src/use-transcription.ts`:
- Around line 33-36: Update the transcription hook’s options and return types to
extend the shared UseGenerationOptions and UseGenerationReturn types, removing
the duplicated persistence, initialResumeSnapshot, and resume-related fields.
Follow the existing Solid generation hook pattern while preserving the hook’s
transcription-specific fields and behavior.
In `@packages/ai-solid/src/use-generate-audio.ts`:
- Around line 59-72: Remove the redundant result, isLoading, error, and status
members from UseGenerateAudioReturn, retaining only the generate override whose
input type differs. Continue inheriting the unchanged members through
Omit<UseGenerationReturn<TOutput>, 'generate'>.
In `@packages/ai-solid/src/use-generate-video.ts`:
- Around line 256-257: Replace the garbled “—” sequence in the mount/devtools
comment with the intended em dash “—”, and apply the same comment-text
correction in the corresponding occurrence in use-generation.ts.
In `@packages/ai-solid/src/use-generation.ts`:
- Around line 223-224: Replace the garbled em dash in the mount-devtools comment
near the generation flow with the correct dash character, matching the
corresponding comment in use-generate-video.ts.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c51b7a1b-4fb8-4467-8a7a-defc34ae7003
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (72)
.changeset/generation-persistence.mddocs/config.jsondocs/persistence/generation-persistence.mdexamples/ts-react-chat/src/routes/generations.image.tsxpackages/ai-angular/src/inject-generate-audio.tspackages/ai-angular/src/inject-generate-image.tspackages/ai-angular/src/inject-generate-speech.tspackages/ai-angular/src/inject-generate-video.tspackages/ai-angular/src/inject-generation.tspackages/ai-angular/src/inject-summarize.tspackages/ai-angular/src/inject-transcription.tspackages/ai-angular/tests/inject-generation.test.tspackages/ai-client/src/generation-client.tspackages/ai-client/src/generation-types.tspackages/ai-client/src/index.tspackages/ai-client/src/storage-adapters.tspackages/ai-client/src/video-generation-client.tspackages/ai-client/tests/generation-client.test.tspackages/ai-client/tests/generation-resume-state.test.tspackages/ai-event-client/src/index.tspackages/ai-persistence/package.jsonpackages/ai-persistence/src/index.tspackages/ai-persistence/src/memory.tspackages/ai-persistence/src/middleware.tspackages/ai-persistence/src/retrieve.tspackages/ai-persistence/src/types.tspackages/ai-persistence/tests/generation-artifacts.test.tspackages/ai-persistence/tests/memory.test.tspackages/ai-persistence/tests/persistence-types.test-d.tspackages/ai-react/src/use-generate-audio.tspackages/ai-react/src/use-generate-image.tspackages/ai-react/src/use-generate-speech.tspackages/ai-react/src/use-generate-video.tspackages/ai-react/src/use-generation.tspackages/ai-react/src/use-summarize.tspackages/ai-react/src/use-transcription.tspackages/ai-react/tests/use-generation.test.tspackages/ai-solid/src/use-generate-audio.tspackages/ai-solid/src/use-generate-image.tspackages/ai-solid/src/use-generate-speech.tspackages/ai-solid/src/use-generate-video.tspackages/ai-solid/src/use-generation.tspackages/ai-solid/src/use-summarize.tspackages/ai-solid/src/use-transcription.tspackages/ai-solid/tests/use-generation.test.tspackages/ai-svelte/src/create-generate-audio.svelte.tspackages/ai-svelte/src/create-generate-image.svelte.tspackages/ai-svelte/src/create-generate-speech.svelte.tspackages/ai-svelte/src/create-generate-video.svelte.tspackages/ai-svelte/src/create-generation.svelte.tspackages/ai-svelte/src/create-summarize.svelte.tspackages/ai-svelte/src/create-transcription.svelte.tspackages/ai-svelte/tests/create-generation.test.tspackages/ai-utils/src/base64.tspackages/ai-utils/src/index.tspackages/ai-vue/src/use-generate-audio.tspackages/ai-vue/src/use-generate-image.tspackages/ai-vue/src/use-generate-speech.tspackages/ai-vue/src/use-generate-video.tspackages/ai-vue/src/use-generation.tspackages/ai-vue/src/use-summarize.tspackages/ai-vue/src/use-transcription.tspackages/ai-vue/tests/use-generation.test.tspackages/ai/src/activities/generateAudio/index.tspackages/ai/src/activities/generateImage/index.tspackages/ai/src/activities/generateSpeech/index.tspackages/ai/src/activities/generateTranscription/index.tspackages/ai/src/activities/middleware/index.tspackages/ai/src/activities/middleware/run.tspackages/ai/src/activities/middleware/types.tspackages/ai/src/activities/stream-generation-result.tspackages/ai/src/index.ts
Comment on lines
+147
to
+165
| // Serve a stored artifact's bytes by id. | ||
| export async function GET(request: Request) { | ||
| const artifactId = new URL(request.url).searchParams.get('id') | ||
| if (!artifactId) return new Response('missing id', { status: 400 }) | ||
|
|
||
| const artifact = await retrieveArtifact(persistence, artifactId) | ||
| if (!artifact) return new Response('not found', { status: 404 }) | ||
|
|
||
| const blob = await retrieveBlob(persistence, artifact) | ||
| if (!blob) return new Response('not found', { status: 404 }) | ||
|
|
||
| return new Response(blob.body ?? (await blob.arrayBuffer()), { | ||
| headers: { | ||
| 'content-type': artifact.mimeType, | ||
| 'content-length': String(artifact.size), | ||
| }, | ||
| }) | ||
| } | ||
| ``` |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Artifact GET route has no authorization check.
The example serves stored bytes for any artifactId supplied in the query string, with no check that the caller is entitled to read that artifact. Since this doc is likely copied verbatim, this teaches an IDOR-prone pattern: anyone who obtains or guesses an artifactId can fetch another user's/thread's generated media.
Add a note (or code) showing that production routes must authorize the artifact against the caller's session/thread ownership before calling retrieveArtifact/retrieveBlob.
🔒 Suggested addition
export async function GET(request: Request) {
const artifactId = new URL(request.url).searchParams.get('id')
if (!artifactId) return new Response('missing id', { status: 400 })
const artifact = await retrieveArtifact(persistence, artifactId)
if (!artifact) return new Response('not found', { status: 404 })
+ // Authorize: verify the artifact's runId/threadId belongs to the
+ // requesting session before serving its bytes.
+ if (!(await isOwnedByCurrentUser(request, artifact))) {
+ return new Response('forbidden', { status: 403 })
+ }
const blob = await retrieveBlob(persistence, artifact)Based on learnings, "do not rely on client-provided [opaque identifiers] as an authorization mechanism ... derive the effective user/session identity from server-side session state, then authorize ... before any persistence reads."
🤖 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 `@docs/persistence/generation-persistence.md` around lines 147 - 165, The
documented artifact GET example reads and serves bytes without verifying caller
authorization. Update the GET handler before retrieveArtifact to derive the
caller identity from trusted server-side session state and authorize access to
artifactId against the owning user or thread; only perform retrieveArtifact and
retrieveBlob after authorization, and clearly mark this check as required for
production routes.
Source: Learnings
| re-attaches on its own through `fetchServerSentEvents` or `fetchHttpStream`, the | ||
| same adapters `useChat` uses. | ||
|
|
||
| A full page reload is different. The hooks do not start a run on mount, so they |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Minor grammar: hyphenate compound adjective.
"A full page reload is different" → "A full-page reload is different."
🧰 Tools
🪛 LanguageTool
[uncategorized] ~237-~237: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ..., the same adapters useChat` uses. A full page reload is different. The hooks do not s...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🤖 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 `@docs/persistence/generation-persistence.md` at line 237, Update the sentence
beginning “A full page reload” to hyphenate the compound adjective as “A
full-page reload,” leaving the rest of the sentence unchanged.
Source: Linters/SAST tools
Comment on lines
+217
to
+218
| // Mount devtools only. Generation runs are never auto-started after render — | ||
| // persisted state is read-only for display. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mojibake em dash ("—") in newly-added comments/docs. Both sites introduce the same broken UTF-8-as-Latin1 em-dash artifact in new comment text; replace with a proper em dash (—).
packages/ai-angular/src/inject-generation.ts#L217-L218: fix"never auto-started after render —"to use a proper em dash.packages/ai-react/src/use-generate-audio.ts#L83-L88: fix the two—occurrences in theuseGenerateAudioJSDoc transport-mode bullets.
📍 Affects 2 files
packages/ai-angular/src/inject-generation.ts#L217-L218(this comment)packages/ai-react/src/use-generate-audio.ts#L83-L88
🤖 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/ai-angular/src/inject-generation.ts` around lines 217 - 218, Replace
the mojibake em-dash in the comment near the devtools-only generation logic in
packages/ai-angular/src/inject-generation.ts (lines 217-218) with a proper em
dash; also replace both mojibake occurrences in the useGenerateAudio
transport-mode JSDoc in packages/ai-react/src/use-generate-audio.ts (lines
83-88), without changing the surrounding wording.
Comment on lines
176
to
179
| async generate(input: TInput): Promise<void> { | ||
| this.mountDevtools() | ||
| if (this.disposed) return | ||
| if (this.isLoading) return |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
mountDevtools() runs before the disposed check.
Calling generate() after dispose() re-registers devtools (devtoolsMounted was reset to false in dispose()) before the disposed guard returns, re-invoking emitRegistered()/emitSnapshot() on an already-disposed bridge.
🔧 Suggested fix
async generate(input: TInput): Promise<void> {
- this.mountDevtools()
- if (this.disposed) return
+ if (this.disposed) return
+ this.mountDevtools()
if (this.isLoading) return📝 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
| async generate(input: TInput): Promise<void> { | |
| this.mountDevtools() | |
| if (this.disposed) return | |
| if (this.isLoading) return | |
| async generate(input: TInput): Promise<void> { | |
| if (this.disposed) return | |
| this.mountDevtools() | |
| if (this.isLoading) return |
🤖 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/ai-client/src/generation-client.ts` around lines 176 - 179, Move the
disposed guard in generate ahead of mountDevtools so calls after disposal return
without re-registering devtools or emitting bridge events. Preserve the existing
isLoading guard and normal devtools mounting for active clients.
Comment on lines
+537
to
+548
| private completePlainFetcherResumeSnapshot(): void { | ||
| if (!this.resumeSnapshot) { | ||
| return | ||
| } | ||
| this.resumeSnapshot = { | ||
| ...this.resumeSnapshot, | ||
| resumeState: null, | ||
| status: 'complete', | ||
| } | ||
| this.callbacksRef.onResumeSnapshotChange?.(this.resumeSnapshot) | ||
| void this.persistResumeSnapshot(this.resumeSnapshot) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fetcher-path completions never create a resume snapshot on the first run.
completePlainFetcherResumeSnapshot() is the only completion hook for the direct fetcher (non-streaming) path (called from generate()), but it no-ops when this.resumeSnapshot is undefined. Since the fetcher path never goes through processStream/observeResumeSnapshot, resumeSnapshot stays undefined unless an initialResumeSnapshot was supplied — so a client configured with persistence + fetcher and no seed snapshot never persists anything for a completed run. This silently breaks the persistence option for the entire fetcher use case, and isn't covered by the new "resume snapshot persistence" tests (which all use connection).
🐛 Suggested fix
private completePlainFetcherResumeSnapshot(): void {
- if (!this.resumeSnapshot) {
- return
- }
this.resumeSnapshot = {
- ...this.resumeSnapshot,
+ ...(this.resumeSnapshot ?? {}),
resumeState: null,
status: 'complete',
}
this.callbacksRef.onResumeSnapshotChange?.(this.resumeSnapshot)
void this.persistResumeSnapshot(this.resumeSnapshot)
}Worth checking whether VideoGenerationClient's fetcher path has the same gap.
📝 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
| private completePlainFetcherResumeSnapshot(): void { | |
| if (!this.resumeSnapshot) { | |
| return | |
| } | |
| this.resumeSnapshot = { | |
| ...this.resumeSnapshot, | |
| resumeState: null, | |
| status: 'complete', | |
| } | |
| this.callbacksRef.onResumeSnapshotChange?.(this.resumeSnapshot) | |
| void this.persistResumeSnapshot(this.resumeSnapshot) | |
| } | |
| private completePlainFetcherResumeSnapshot(): void { | |
| this.resumeSnapshot = { | |
| ...(this.resumeSnapshot ?? {}), | |
| resumeState: null, | |
| status: 'complete', | |
| } | |
| this.callbacksRef.onResumeSnapshotChange?.(this.resumeSnapshot) | |
| void this.persistResumeSnapshot(this.resumeSnapshot) | |
| } |
🤖 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/ai-client/src/generation-client.ts` around lines 537 - 548, Update
completePlainFetcherResumeSnapshot() to create a new completed resume snapshot
when this.resumeSnapshot is undefined, using the client’s established snapshot
shape and current generation context. Preserve existing seeded-snapshot updates,
callback notification, and persistence behavior, and inspect
VideoGenerationClient’s analogous fetcher completion hook for the same missing
initialization.
Comment on lines
+46
to
+53
| export interface WithPersistenceOptions { | ||
| extractArtifacts?: ( | ||
| input: GenerationArtifactExtractionInput, | ||
| ) => | ||
| | Array<GenerationArtifactDescriptor | PersistedArtifactRef> | ||
| | Promise<Array<GenerationArtifactDescriptor | PersistedArtifactRef>> | ||
| nameArtifact?: (input: GenerationArtifactNameInput) => string | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
WithPersistenceOptions is declared twice in this file (interface merging).
Line 916 declares another export interface WithPersistenceOptions with snapshotStreaming / snapshotIntervalMs. TypeScript merges them, so both withPersistence and withGenerationPersistence silently accept the union of chat-snapshot and artifact options. If that is intentional, fold the new members into the existing declaration; if not, give the generation entrypoint its own option type (e.g. WithGenerationPersistenceOptions) so the two surfaces stay distinct.
🤖 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/ai-persistence/src/middleware.ts` around lines 46 - 53, Resolve the
duplicate WithPersistenceOptions declarations by keeping chat persistence
options separate from generation persistence options: rename the
generation-specific declaration and update withGenerationPersistence to use it,
or consolidate only if both APIs intentionally share all options. Ensure
withPersistence and withGenerationPersistence no longer accidentally accept each
other’s option fields.
Comment on lines
+332
to
+345
| function parseDataUrl( | ||
| value: string, | ||
| ): { mimeType: string; bytes: Uint8Array } | undefined { | ||
| const match = /^data:([^;,]+)?(;base64)?,(.*)$/s.exec(value) | ||
| if (!match) return undefined | ||
| const mimeType = match[1] || 'application/octet-stream' | ||
| const payload = decodeURIComponent(match[3] ?? '') | ||
| return { | ||
| mimeType, | ||
| bytes: match[2] | ||
| ? base64ToUint8Array(payload) | ||
| : new TextEncoder().encode(payload), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
parseDataUrl rejects data URLs carrying extra media-type parameters.
The regex only allows ;base64 immediately after the MIME type, so data:audio/wav;codecs=1;base64,… or data:text/plain;charset=utf-8,… fails to match. The consequences are silent: line 546 then feeds the whole data: string to base64ToUint8Array, persisting garbage bytes, and line 626 tries to fetch() a data: URL.
Parse the parameter list generically instead.
🐛 Proposed fix
- const match = /^data:([^;,]+)?(;base64)?,(.*)$/s.exec(value)
+ const match = /^data:([^,]*),(.*)$/s.exec(value)
if (!match) return undefined
- const mimeType = match[1] || 'application/octet-stream'
- const payload = decodeURIComponent(match[3] ?? '')
+ const meta = match[1] ?? ''
+ const params = meta.split(';')
+ const isBase64 = params.at(-1)?.trim().toLowerCase() === 'base64'
+ const mimeType = params[0] || 'application/octet-stream'
+ const raw = match[2] ?? ''
+ const payload = isBase64 ? raw.trim() : decodeURIComponent(raw)
return {
mimeType,
- bytes: match[2]
+ bytes: isBase64
? base64ToUint8Array(payload)
: new TextEncoder().encode(payload),
}📝 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
| function parseDataUrl( | |
| value: string, | |
| ): { mimeType: string; bytes: Uint8Array } | undefined { | |
| const match = /^data:([^;,]+)?(;base64)?,(.*)$/s.exec(value) | |
| if (!match) return undefined | |
| const mimeType = match[1] || 'application/octet-stream' | |
| const payload = decodeURIComponent(match[3] ?? '') | |
| return { | |
| mimeType, | |
| bytes: match[2] | |
| ? base64ToUint8Array(payload) | |
| : new TextEncoder().encode(payload), | |
| } | |
| } | |
| function parseDataUrl( | |
| value: string, | |
| ): { mimeType: string; bytes: Uint8Array } | undefined { | |
| const match = /^data:([^,]*),(.*)$/s.exec(value) | |
| if (!match) return undefined | |
| const meta = match[1] ?? '' | |
| const params = meta.split(';') | |
| const isBase64 = params.at(-1)?.trim().toLowerCase() === 'base64' | |
| const mimeType = params[0] || 'application/octet-stream' | |
| const raw = match[2] ?? '' | |
| const payload = isBase64 ? raw.trim() : decodeURIComponent(raw) | |
| return { | |
| mimeType, | |
| bytes: isBase64 | |
| ? base64ToUint8Array(payload) | |
| : new TextEncoder().encode(payload), | |
| } | |
| } |
🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 335-335: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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/ai-persistence/src/middleware.ts` around lines 332 - 345, Update
parseDataUrl to parse the data URL’s media type and semicolon-delimited
parameters generically, allowing parameters such as codecs or charset before an
optional final base64 marker. Return the MIME type without parameters, decode
the payload according to whether base64 is present, and preserve undefined for
invalid data URLs so callers do not process the original data: string.
Comment on lines
+617
to
+655
| if (descriptor.url) { | ||
| const data = parseDataUrl(descriptor.url) | ||
| if (data) { | ||
| return { | ||
| body: data.bytes, | ||
| size: data.bytes.byteLength, | ||
| mimeType: descriptor.mimeType ?? data.mimeType, | ||
| } | ||
| } | ||
| const response = await fetch(descriptor.url) | ||
| if (!response.ok) { | ||
| throw new Error( | ||
| `Failed to persist artifact from ${descriptor.url}: HTTP ${response.status}`, | ||
| ) | ||
| } | ||
| const mimeType = | ||
| descriptor.mimeType ?? | ||
| response.headers.get('content-type') ?? | ||
| 'application/octet-stream' | ||
| // Stream the body straight into the blob store instead of buffering the | ||
| // whole artifact in memory. `size` is left 0 (unknown up front); the store | ||
| // records the actual byte length as it drains the stream. Fall back to | ||
| // buffering only when the response has no body to stream. | ||
| if (response.body) { | ||
| return { | ||
| body: response.body, | ||
| size: 0, | ||
| mimeType, | ||
| externalUrl: descriptor.url, | ||
| } | ||
| } | ||
| const body = await response.arrayBuffer() | ||
| return { | ||
| body, | ||
| size: body.byteLength, | ||
| mimeType, | ||
| externalUrl: descriptor.url, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Unbounded server-side fetch of a model/prompt-supplied URL — SSRF and no timeout.
descriptor.url reaches here from built-in extraction of prompt parts (sourcePartDescriptors, url source values) and provider output, i.e. potentially attacker-influenced input. The server then fetches it with no scheme/host validation, no redirect policy, no size cap, and no timeout, and streams the response into durable storage. That allows internal-network probing/exfiltration into the blob store, plus a request thread that can hang indefinitely or store an unbounded payload.
At minimum: restrict to http/https, apply an allowlist or SSRF guard hook, pass an AbortSignal timeout, and cap the persisted byte length.
🔒 Sketch of the guard
- const response = await fetch(descriptor.url)
+ const target = new URL(descriptor.url)
+ if (target.protocol !== 'https:' && target.protocol !== 'http:') {
+ throw new Error(`Refusing to fetch artifact from ${target.protocol}`)
+ }
+ // Callers should be able to veto hosts (private ranges, metadata IPs, …).
+ await assertFetchableArtifactUrl?.(target)
+ const response = await fetch(target, {
+ redirect: 'error',
+ signal: AbortSignal.timeout(artifactFetchTimeoutMs ?? 30_000),
+ })🤖 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/ai-persistence/src/middleware.ts` around lines 617 - 655, Harden URL
handling in the descriptor.url fetch path: validate and allow only http/https
URLs, invoke the existing allowlist or SSRF guard hook before fetch, and use an
AbortSignal timeout. Enforce a maximum response size for both streamed and
buffered bodies, aborting or rejecting oversized responses before persisting
them; preserve data-URL handling and metadata behavior.
Comment on lines
+14
to
+45
| /** | ||
| * Look up a persisted generation artifact's metadata by id. Returns `null` when | ||
| * the persistence has no `artifacts` store or no record matches — so a serve | ||
| * handler can map that straight to a 404. | ||
| */ | ||
| export async function retrieveArtifact( | ||
| persistence: AIPersistence, | ||
| artifactId: string, | ||
| ): Promise<ArtifactRecord | null> { | ||
| const record = await persistence.stores.artifacts?.get(artifactId) | ||
| return record ?? null | ||
| } | ||
|
|
||
| /** | ||
| * Look up a persisted generation artifact's stored bytes. Pass an `artifactId` | ||
| * (resolved to its record first) or an already-loaded {@link ArtifactRecord} | ||
| * (no second metadata lookup). Returns `null` when the artifact, its record, or | ||
| * its blob is missing, or the stores are not configured. | ||
| */ | ||
| export async function retrieveBlob( | ||
| persistence: AIPersistence, | ||
| artifact: string | ArtifactRecord, | ||
| ): Promise<BlobObject | null> { | ||
| const record = | ||
| typeof artifact === 'string' | ||
| ? await retrieveArtifact(persistence, artifact) | ||
| : artifact | ||
| if (!record) return null | ||
|
|
||
| const blob = await persistence.stores.blobs?.get(artifactBlobKey(record)) | ||
| return blob ?? null | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Document that callers must authorize before serving artifact bytes.
retrieveArtifact/retrieveBlob key purely on a client-supplied artifactId and return the record's threadId/runId without any ownership check. The JSDoc suggests wiring the null case straight to a 404 in a serve handler, which invites an IDOR if the handler skips authorization. Add a line telling callers to derive identity server-side and verify record.threadId ownership before returning bytes.
Based on learnings: treat identifiers as opaque lookup keys and authorize thread ownership at the route boundary before any persistence read.
🤖 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/ai-persistence/src/retrieve.ts` around lines 14 - 45, Update the
JSDoc for retrieveArtifact and retrieveBlob to state that artifact identifiers
are opaque lookup keys and callers must derive identity server-side and verify
record.threadId ownership at the route boundary before serving bytes. Remove or
qualify wording that suggests mapping a null result directly to a 404 without
authorization.
Source: Learnings
Comment on lines
187
to
+198
| onResult: ((r: TResult) => options.onResult?.(r)) as ( | ||
| result: TResult, | ||
| ) => TOutput | null | void, | ||
| onError: (e: Error) => options.onError?.(e), | ||
| onProgress: (p: number, m?: string) => options.onProgress?.(p, m), | ||
| onChunk: (c: StreamChunk) => options.onChunk?.(c), | ||
| onError: (e: Error) => { | ||
| if (!disposed) options.onError?.(e) | ||
| }, | ||
| onProgress: (p: number, m?: string) => { | ||
| if (!disposed) options.onProgress?.(p, m) | ||
| }, | ||
| onChunk: (c: StreamChunk) => { | ||
| if (!disposed) options.onChunk?.(c) | ||
| }, |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
onResult callback missing the new disposed guard in both useGeneration and useGenerateVideo. This PR wraps onError/onProgress/onChunk (and the internal on*Change state setters) with if (!disposed) checks to stop firing after onScopeDispose, but the sibling public user callback onResult was left unguarded in both places — the shared root cause is one missing guard pattern applied to a new callback category.
packages/ai-vue/src/use-generation.ts#L187-L198: wrap theonResultcallback body with the sameif (disposed) returnguard used foronError/onProgress/onChunkjust below it.packages/ai-vue/src/use-generate-video.ts#L203-L214: apply the identical guard toonResultinbaseOptions, matchingonError/onProgress/onChunk.
📍 Affects 2 files
packages/ai-vue/src/use-generation.ts#L187-L198(this comment)packages/ai-vue/src/use-generate-video.ts#L203-L214
🤖 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/ai-vue/src/use-generation.ts` around lines 187 - 198, Guard the
public onResult callbacks against disposed scopes, matching the existing
onError/onProgress/onChunk behavior. In packages/ai-vue/src/use-generation.ts
lines 187-198, return without invoking options.onResult when disposed; apply the
identical change to the baseOptions onResult callback in
packages/ai-vue/src/use-generate-video.ts lines 203-214.
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.
Generation Persistence — client snapshot + durable media bytes
Targets
main(the persistence base, #984, has merged; this was previously stacked onfeat/persistence-core, now rebased ontomain).Generation persistence in two halves.
1. Client resume snapshot (read-only)
As a media generation streams, the client builds a lightweight
GenerationResumeSnapshot(run identity, status, errors, result metadata + artifact refs — never the bytes) and writes it to apersistencestorage adapter. The option reuses the sharedChatStorageAdaptercontract, solocalStoragePersistence/sessionStoragePersistence/indexedDBPersistencework with no type argument:Hooks expose
resumeSnapshot/resumeState/pendingArtifacts/resultArtifactsacross react/solid/vue/svelte/angular. Noresume()action; reconnecting to an in-flight stream is the delivery layer's job (resumable streams), wired via adurabilityadapter +GEThandler exactly as for chat.2. Durable media-byte storage (server, opt-in)
When the persistence backend provides both an
artifacts(ArtifactStore) and ablobs(BlobStore) store,withGenerationPersistencewrites each generated file's bytes to the blob store (keyartifacts/<runId>/<artifactId>), records anArtifactRecord, attachesPersistedArtifactRefs to the result, and emitsgeneration:artifactsso the client records them.memoryPersistence()ships both stores; any backend implementing the two contracts works. Extraction is customizable viaextractArtifacts/nameArtifact. Serve the bytes back with theretrieveArtifact/retrieveBlobhelpers (and the sharedartifactBlobKey).Changes
@tanstack/ai— result-transform machinery (resultTransforms/artifactInputsonGenerationMiddlewareContext,applyGenerationResultTransforms),threadId/runIdoptions on the image/audio/speech/transcription activities, andgeneration:artifactsemission fromstreamGenerationResult.@tanstack/ai-utils—base64ToUint8Array.@tanstack/ai-persistence—ArtifactStore+BlobStorecontracts, in-memory impls inmemoryPersistence(), byte-persistence inwithGenerationPersistence, andretrieveArtifact/retrieveBlob/artifactBlobKeyserve helpers.@tanstack/ai-client+ framework hooks (react/solid/vue/svelte/angular) — the client snapshot +persistenceoption + artifact refs.@tanstack/ai-event-client— optionalthreadId/runIdon generation events.docs/persistence/generation-persistence.md, kiira-verified.Verification
test:types— 0 errors in ai / ai-client / ai-persistence / all 5 frameworks.Scope note
Durable SQL/R2 artifact+blob backends (Drizzle/Prisma/Cloudflare R2) are not in this PR —
memoryPersistenceplus any customArtifactStore+BlobStorecover the contract today; the durable backends (SQL schema + migrations per package) are a clean follow-up.generateVideo's own job-polling artifact path is likewise deferred (image/audio/speech/transcription persist bytes).Summary by CodeRabbit
New Features
Bug Fixes