feat: concurrent single generation queue with cancellation support#30
Merged
Simplereally merged 4 commits intomainfrom Mar 3, 2026
Merged
feat: concurrent single generation queue with cancellation support#30Simplereally merged 4 commits intomainfrom
Simplereally merged 4 commits intomainfrom
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds per-generation queueing and cancellation across frontend, hook, and backend: a new exported Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as "Studio UI"
participant Hook as "useGenerateImage"
participant Convex as "Convex Backend"
participant Canvas as "ImageCanvas"
participant R2 as "R2 Storage"
User->>UI: Click Generate (while another generation active)
UI->>Hook: generate(params)
Hook->>Convex: startGeneration mutation
Convex-->>Hook: generationId
Hook->>Hook: enqueue ActiveGenerationEntry (currentGenerationId updated)
Hook->>Convex: poll getGenerationsStatus([ids...])
Convex-->>Hook: statuses for ids
Hook-->>UI: expose queueItems + onCancelItem
UI->>Canvas: render queue cards with cancel buttons
User->>Canvas: Click cancel on a queue card
Canvas->>Hook: onCancelItem(generationId)
Hook->>Convex: cancelGeneration(generationId)
Convex->>Convex: validate + set status "cancelled"
Convex-->>Hook: { success: true }
alt If generation had R2 objects
Convex->>R2: deleteR2Objects([keys])
R2-->>Convex: deletions settled
end
Hook->>Hook: remove entry, reject promise with CANCELLED
Hook-->>UI: updated queueItems
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee2abcc5b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+237
to
+239
| const generatedImage = await convex.query(api.generatedImages.getById, { | ||
| imageId: generationStatus.imageId, | ||
| }) |
There was a problem hiding this comment.
When a generation reaches completed, this convex.query(...) call can reject (for example on a transient client/network/auth error). That exception bypasses both entry.resolve and entry.reject, but the finally path still removes the generation from local tracking, so the queue item disappears and generateAsync can remain pending forever. Catch and convert this path into a rejected generation before removing it from activeGenerations.
Useful? React with 👍 / 👎.
Comment on lines
+307
to
+309
| setCurrentGenerationId((prev) => | ||
| prev === generationId ? null : prev | ||
| ) |
There was a problem hiding this comment.
In concurrent mode, if the most recently started generation finishes before an older one, this branch sets currentGenerationId to null even though activeGenerations still contains in-flight work. In that state, cancelCurrentGeneration() becomes a no-op while isGenerating is still true, which breaks cancellation behavior for callers that rely on the "current" generation handle.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hooks/queries/use-generate-image.ts (1)
436-449:⚠️ Potential issue | 🟠 Major
reset()reports cancellation but does not cancel server work.Local rejection + state clear without a cancel mutation can leave backend generations running (and charging) after the UI says they were cancelled.
🐛 Suggested server cancellation on reset
- const reset = React.useCallback(() => { + const reset = React.useCallback(() => { for (const entry of activeGenerationsRef.current) { + void cancelGeneration({ generationId: entry.id }).catch(() => {}) entry.reject?.( new ServerGenerationError("Generation cancelled", "CANCELLED") ) } setActiveGenerations([]) setCurrentGenerationId(null) setIsSuccess(false) setIsError(false) setError(null) setData(undefined) - }, []) + }, [cancelGeneration])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/queries/use-generate-image.ts` around lines 436 - 449, The reset function currently rejects local promises via ServerGenerationError but doesn't cancel server-side work; update reset (and use activeGenerationsRef.current) to invoke the server cancellation for each active generation (e.g., call the existing cancel mutation or API with the generation id — cancelGeneration(entry.id) or dispatch cancelGenerationMutation) before or alongside calling entry.reject, await or reliably fire-and-forget the cancel calls and handle/log any cancellation errors, then proceed to clear state (setActiveGenerations, setCurrentGenerationId, setIsSuccess, setIsError, setError, setData) as currently implemented.
🧹 Nitpick comments (6)
lib/storage/r2-client.test.ts (1)
36-40: Remove unsafe type assertions in crypto mocks.The casts on
randomUUIDandcreateHashweaken strict typing and can mask mock/interface mismatches. Use a real UUID literal and avoid force-casting a partial object tocrypto.Hash.Suggested adjustment
const mockRandomUUID = vi.spyOn(crypto, "randomUUID").mockReturnValue( - "mock-uuid" as `${string}-${string}-${string}-${string}-${string}` + "00000000-0000-4000-8000-000000000000" ); -const mockCreateHash = vi.spyOn(crypto, "createHash").mockReturnValue({ - update: vi.fn().mockReturnThis(), - digest: vi.fn().mockReturnValue("mock-user-hash"), -} as unknown as crypto.Hash); +// Prefer not mocking createHash; assert against deterministic real hash in test expectations.As per coding guidelines, "Avoid
ascasts in TypeScript; prefersatisfies, generics, and runtime validation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage/r2-client.test.ts` around lines 36 - 40, The tests currently use unsafe "as" casts for crypto.randomUUID and crypto.createHash (mockRandomUUID and mockCreateHash); replace the fake UUID cast with a real UUID string literal (e.g. "00000000-0000-0000-0000-000000000000") when mocking crypto.randomUUID, and replace the partial object cast for crypto.createHash with a properly typed mock that satisfies the crypto.Hash shape (either create a tiny class implementing update/digest that implements crypto.Hash or use TypeScript's "satisfies crypto.Hash" on the object) so you avoid force-casting and preserve strict typing for createHash and randomUUID.components/studio/layout/studio-shell.test.tsx (1)
262-269: Cancellation mock is added but not assertion-tested.
cancelGenerationByIdis now mocked, but there’s no test in this file that proves the cancel path is invoked. Add one focused test to trigger queue-item cancel and assertmockCancelGenerationByIdwas called.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/studio/layout/studio-shell.test.tsx` around lines 262 - 269, Add a focused test that verifies the cancel path by using the existing mockCancelGenerationById from the vi.mock of useGenerateImage: render the StudioShell component with a queued generation item, locate the queue-item cancel control (e.g., the cancel button or menu action tied to the queue item) and simulate a user click, then assert mockCancelGenerationById was called with the expected generation id; ensure the test imports/uses the same mock symbol (mockCancelGenerationById) and resets/clears the mock between tests to avoid cross-test pollution.convex/singleGenerationProcessor.ts (1)
223-231: Consider error handling for R2 cleanup.The best-effort R2 cleanup is appropriate, but
deleteR2Objectsfailures are silently ignored. If cleanup fails, orphaned objects may remain in R2.Consider wrapping in try-catch with logging to aid debugging orphaned assets:
💡 Suggested improvement
if (await isCancelled()) { console.log(`${logger} Generation ${args.generationId} was cancelled before persistence, cleaning up R2 objects`) // Best-effort R2 cleanup to avoid orphan objects const keysToDelete = [r2Key] if (thumbnailResult?.url) keysToDelete.push(generateThumbnailKey(r2Key)) if (previewResult?.url) keysToDelete.push(generatePreviewKey(r2Key)) - await deleteR2Objects(keysToDelete) + try { + await deleteR2Objects(keysToDelete) + console.log(`${logger} R2 cleanup successful for cancelled generation ${args.generationId}`) + } catch (cleanupError) { + console.warn(`${logger} R2 cleanup failed for cancelled generation ${args.generationId}:`, cleanupError) + } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/singleGenerationProcessor.ts` around lines 223 - 231, The R2 cleanup call can fail silently; wrap the best-effort cleanup block (the isCancelled() branch that builds keysToDelete using r2Key, generateThumbnailKey(r2Key), generatePreviewKey(r2Key)) in a try-catch, call deleteR2Objects inside the try, and on catch log the error with process/context info using the same logger (include args.generationId and the keysToDelete) so failures to delete R2 objects are recorded for debugging and monitoring.components/studio/canvas/image-canvas.test.tsx (1)
26-33: Remove the status type cast in the queue-item factory.The cast on
statusweakens strict typing here and is avoidable with a typed local variable.♻️ Suggested update
const createQueueItems = (count: number): QueueItem[] => - Array.from({ length: count }, (_, i) => ({ - id: `gen-${i + 1}`, - status: (i === 0 ? "processing" : "pending") as "processing" | "pending", - createdAt: Date.now() - (count - i) * 1000, - aspectRatio: 1, - labelIndex: i + 1, - })) + Array.from({ length: count }, (_, i) => { + const status: QueueItem["status"] = i === 0 ? "processing" : "pending" + return { + id: `gen-${i + 1}`, + status, + createdAt: Date.now() - (count - i) * 1000, + aspectRatio: 1, + labelIndex: i + 1, + } + })As per coding guidelines: "Avoid
ascasts in TypeScript; prefersatisfies, generics, and runtime validation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/studio/canvas/image-canvas.test.tsx` around lines 26 - 33, The status field in createQueueItems is currently forced with an `as` cast; remove that cast and introduce a properly typed local variable (e.g., `const status: QueueItem['status'] = i === 0 ? "processing" : "pending"`) inside the createQueueItems arrow function, then use that variable for the status property so the factory preserves strict typing without using `as`; update the function signature/return to continue returning QueueItem[] and ensure no other casts remain in createQueueItems.hooks/queries/use-generate-image.test.tsx (1)
36-44: Make mockeduseMutationdispatch by API ref, not call order.Modulo-index dispatch can silently wire the wrong mutation when hook internals change. Keying by
mutationRefis safer and helps eliminate several cast workarounds in this file.♻️ Suggested mock wiring
-let mutationCallIndex = 0 vi.mock("convex/react", () => ({ - useMutation: () => { - const fns = [mockStartGeneration, mockCancelGeneration] - const fn = fns[mutationCallIndex % 2] - mutationCallIndex++ - return fn - }, + useMutation: (mutationRef: unknown) => { + if (mutationRef === "singleGeneration.startGeneration") return mockStartGeneration + if (mutationRef === "singleGeneration.cancelGeneration") return mockCancelGeneration + throw new Error(`Unexpected mutation ref: ${String(mutationRef)}`) + },As per coding guidelines: "Avoid
ascasts in TypeScript; prefersatisfies, generics, and runtime validation."Also applies to: 53-54, 67-70, 453-478
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/queries/use-generate-image.test.tsx` around lines 36 - 44, The mocked useMutation currently returns functions based on a global mutationCallIndex which is brittle; change the mock implementation of useMutation to inspect the provided mutationRef (e.g., check mutationRef.name or another stable identifier) and return the correct mock (mockStartGeneration or mockCancelGeneration) based on that key instead of call order; update all similar mocks (the spots around lines referenced and uses of mutationCallIndex) to map mutationRef -> mock function and remove the modulo/indexing logic and any related TypeScript casts so the wiring is deterministic and type-safe.components/studio/layout/studio-shell.tsx (1)
249-261: Narrow queue statuses instead of casting them.The status cast bypasses strict typing and can be replaced with a small runtime guard that stays safe if enum values evolve.
♻️ Suggested narrowing
const singleQueueItems: QueueItem[] = React.useMemo(() => { + const isQueueStatus = (status: string): status is QueueItem["status"] => + status === "pending" || status === "processing" + const sorted = [...activeSingleList].sort( (a, b) => a.createdAt - b.createdAt, ); - return sorted.map((g, i) => ({ - id: g._id, - status: g.status as "pending" | "processing", - createdAt: g.createdAt, - aspectRatio: - (g.generationParams?.width ?? 1024) / - (g.generationParams?.height ?? 1024), - labelIndex: i + 1, - })); + return sorted.flatMap((g, i) => + isQueueStatus(g.status) + ? [{ + id: g._id, + status: g.status, + createdAt: g.createdAt, + aspectRatio: + (g.generationParams?.width ?? 1024) / + (g.generationParams?.height ?? 1024), + labelIndex: i + 1, + }] + : [] + ); }, [activeSingleList]);As per coding guidelines: "Avoid
ascasts in TypeScript; prefersatisfies, generics, and runtime validation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/studio/layout/studio-shell.tsx` around lines 249 - 261, The code in the singleQueueItems useMemo is unsafely casting g.status to "pending" | "processing"; replace the cast with a small runtime guard that validates g.status against the allowed values and falls back to a safe default (or maps unknown values to "pending"/"processing" as appropriate). Update the mapping inside the singleQueueItems computation (referencing singleQueueItems, QueueItem, activeSingleList and g.status) to perform a check (e.g. string comparison or a Set lookup) and assign a typed status only when it matches the allowed enum values to avoid using "as" casts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/studio/controls/prompt-section.tsx`:
- Around line 458-462: The EnhanceButton for negative prompts is incorrectly
disabled using the positive prompt state `hasContent`; update the `disabled`
prop to use the negative-prompt-specific state provided by
`subscribeToNegativePrompt` (the variable representing whether the negative
prompt has content, e.g., `negativeHasContent` or similar) so that
`EnhanceButton` (props: `isEnhancing={isEnhancingNegativePrompt}`,
`onEnhance={onEnhanceNegativePrompt}`,
`onCancel={onCancelEnhanceNegativePrompt}`) is enabled whenever the negative
prompt actually contains text.
In `@components/studio/layout/studio-shell.tsx`:
- Around line 257-260: The aspectRatio computation can produce Infinity/NaN when
g.generationParams?.height is 0 or missing; update the calculation in the area
constructing aspectRatio (using g.generationParams?.width and
g.generationParams?.height) to defensively check and fall back to a safe
non-zero default or skip calculation when height is falsy (e.g., coerce height
to a positive number or use a default like 1 or width) before dividing so
aspectRatio never becomes Infinity/NaN and downstream layout (queue card sizing)
remains stable.
In `@convex/lib/r2.ts`:
- Around line 164-181: The deleteR2Objects helper currently calls getS3Client()
directly and can throw (breaking the “best-effort non-throwing” contract); wrap
the client initialization and the Promise.allSettled(...) fan-out in a top-level
try/catch inside deleteR2Objects, catch any errors from getS3Client or the
delete loop, log a descriptive message including the error (using the existing
“[R2 Cleanup]” prefix), and return so the function never throws; keep the
existing per-key try/catch for DeleteObjectCommand failures intact.
In `@convex/singleGeneration.ts`:
- Around line 127-145: The getGenerationsStatus query currently maps
args.generationIds straight to Promise.all(ctx.db.get(...)) which allows
unbounded/duplicated db.get fan-out; before calling ctx.db.get in
getGenerationsStatus, validate and normalize the input by obtaining identity via
ctx.auth.getUserIdentity(), then deduplicate generationIds (e.g. using a Set)
and enforce a safe maximum batch size (reject or truncate input beyond a
configured MAX_IDS) so you only map over a bounded, unique list; then perform
the ctx.db.get calls on that sanitized array and continue filtering by
generation.ownerId === identity.subject.
In `@hooks/queries/use-generate-image.ts`:
- Around line 335-347: Wrap the callbacksRef.current.onMutate?.(params)
invocation in a try/catch inside generate() so that if onMutate throws or
returns a rejected promise you handle it: catch the error, create or wrap into a
ServerGenerationError (or reuse existing error path), call setError(err) and
setIsError(true), invoke callbacksRef.current.onError?.(err, params) and
callbacksRef.current.onSettled?.(undefined, err, params), call
deferred?.reject(err) and return early (do not continue to authorize() or
further processing); ensure the try/catch awaits the onMutate call if it returns
a promise so rejected promises are caught.
- Around line 413-427: The cancelGenerationById function is tearing down local
state unconditionally; change it to await cancelGeneration(...) and inspect its
result (and catch errors) so you only call callbacksRef.current.onSettled,
entry.reject, setActiveGenerations and setCurrentGenerationId when the mutation
response indicates success (e.g., result.success === true); if the backend
returns failure or throws, do not remove or reject the local
activeGenerationsRef entry and instead propagate/return the failure so the
completion flow can still process the generation. Ensure you reference
cancelGenerationById, cancelGeneration, activeGenerationsRef, callbacksRef,
setActiveGenerations and setCurrentGenerationId when making this conditional
update.
---
Outside diff comments:
In `@hooks/queries/use-generate-image.ts`:
- Around line 436-449: The reset function currently rejects local promises via
ServerGenerationError but doesn't cancel server-side work; update reset (and use
activeGenerationsRef.current) to invoke the server cancellation for each active
generation (e.g., call the existing cancel mutation or API with the generation
id — cancelGeneration(entry.id) or dispatch cancelGenerationMutation) before or
alongside calling entry.reject, await or reliably fire-and-forget the cancel
calls and handle/log any cancellation errors, then proceed to clear state
(setActiveGenerations, setCurrentGenerationId, setIsSuccess, setIsError,
setError, setData) as currently implemented.
---
Nitpick comments:
In `@components/studio/canvas/image-canvas.test.tsx`:
- Around line 26-33: The status field in createQueueItems is currently forced
with an `as` cast; remove that cast and introduce a properly typed local
variable (e.g., `const status: QueueItem['status'] = i === 0 ? "processing" :
"pending"`) inside the createQueueItems arrow function, then use that variable
for the status property so the factory preserves strict typing without using
`as`; update the function signature/return to continue returning QueueItem[] and
ensure no other casts remain in createQueueItems.
In `@components/studio/layout/studio-shell.test.tsx`:
- Around line 262-269: Add a focused test that verifies the cancel path by using
the existing mockCancelGenerationById from the vi.mock of useGenerateImage:
render the StudioShell component with a queued generation item, locate the
queue-item cancel control (e.g., the cancel button or menu action tied to the
queue item) and simulate a user click, then assert mockCancelGenerationById was
called with the expected generation id; ensure the test imports/uses the same
mock symbol (mockCancelGenerationById) and resets/clears the mock between tests
to avoid cross-test pollution.
In `@components/studio/layout/studio-shell.tsx`:
- Around line 249-261: The code in the singleQueueItems useMemo is unsafely
casting g.status to "pending" | "processing"; replace the cast with a small
runtime guard that validates g.status against the allowed values and falls back
to a safe default (or maps unknown values to "pending"/"processing" as
appropriate). Update the mapping inside the singleQueueItems computation
(referencing singleQueueItems, QueueItem, activeSingleList and g.status) to
perform a check (e.g. string comparison or a Set lookup) and assign a typed
status only when it matches the allowed enum values to avoid using "as" casts.
In `@convex/singleGenerationProcessor.ts`:
- Around line 223-231: The R2 cleanup call can fail silently; wrap the
best-effort cleanup block (the isCancelled() branch that builds keysToDelete
using r2Key, generateThumbnailKey(r2Key), generatePreviewKey(r2Key)) in a
try-catch, call deleteR2Objects inside the try, and on catch log the error with
process/context info using the same logger (include args.generationId and the
keysToDelete) so failures to delete R2 objects are recorded for debugging and
monitoring.
In `@hooks/queries/use-generate-image.test.tsx`:
- Around line 36-44: The mocked useMutation currently returns functions based on
a global mutationCallIndex which is brittle; change the mock implementation of
useMutation to inspect the provided mutationRef (e.g., check mutationRef.name or
another stable identifier) and return the correct mock (mockStartGeneration or
mockCancelGeneration) based on that key instead of call order; update all
similar mocks (the spots around lines referenced and uses of mutationCallIndex)
to map mutationRef -> mock function and remove the modulo/indexing logic and any
related TypeScript casts so the wiring is deterministic and type-safe.
In `@lib/storage/r2-client.test.ts`:
- Around line 36-40: The tests currently use unsafe "as" casts for
crypto.randomUUID and crypto.createHash (mockRandomUUID and mockCreateHash);
replace the fake UUID cast with a real UUID string literal (e.g.
"00000000-0000-0000-0000-000000000000") when mocking crypto.randomUUID, and
replace the partial object cast for crypto.createHash with a properly typed mock
that satisfies the crypto.Hash shape (either create a tiny class implementing
update/digest that implements crypto.Hash or use TypeScript's "satisfies
crypto.Hash" on the object) so you avoid force-casting and preserve strict
typing for createHash and randomUUID.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (23)
components/studio/canvas/image-canvas.test.tsxcomponents/studio/canvas/image-canvas.tsxcomponents/studio/controls/prompt-section.tsxcomponents/studio/features/canvas/canvas-feature.tsxcomponents/studio/features/canvas/canvas-view.tsxcomponents/studio/features/prompt/prompt-feature.test.tsxcomponents/studio/features/prompt/prompt-feature.tsxcomponents/studio/features/prompt/prompt-view.test.tsxcomponents/studio/features/prompt/prompt-view.tsxcomponents/studio/index.tscomponents/studio/layout/studio-shell.test.tsxcomponents/studio/layout/studio-shell.tsxcomponents/studio/mobile/mobile-studio-navigation.test.tsxcomponents/studio/mobile/mobile-studio-navigation.tsxconvex/lib/index.tsconvex/lib/r2.tsconvex/schema.tsconvex/singleGeneration.tsconvex/singleGenerationProcessor.tshooks/queries/use-generate-image.test.tsxhooks/queries/use-generate-image.tshooks/use-image-edit.tslib/storage/r2-client.test.ts
- Fix negative prompt EnhanceButton enable/disable logic and add coverage\n- Guard onMutate errors, make cancel teardown conditional on backend success, and cancel on reset\n- Make R2 cleanup best-effort non-throwing; bound/dedupe generation status fan-out\n- Add queue aspect ratio guard and tighten related tests
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29cb0239f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+194
to
+196
| activeGenerationIds.length > 0 | ||
| ? { generationIds: activeGenerationIds } | ||
| : "skip" |
There was a problem hiding this comment.
This hook forwards the full activeGenerationIds array to getGenerationsStatus, but the new server query now throws TOO_MANY_IDS once more than 100 IDs are passed (convex/singleGeneration.ts). Because there is no client-side cap/chunking here, users who queue many single generations can make the status query throw and break polling for all in-flight items.
Useful? React with 👍 / 👎.
Comment on lines
+241
to
+243
| if (!generatedImage) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
If a generation is marked completed but getById returns null (for example, the image row is not readable yet or was removed), this branch continues without resolving/rejecting the deferred promise; the finally block still removes the entry from activeGenerations, so generateAsync callers can be left pending forever while the queue item disappears.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
convex/singleGeneration.ts (1)
242-285:⚠️ Potential issue | 🟠 MajorProtect
cancelledfrom being overwritten by later status patches.
updateGenerationStatuscurrently patches blindly. In concurrent scenarios, a user-initiated cancellation can be overwritten by a subsequentstatusupdate from the processor, breaking cancellation semantics. For example, afterisCancelled()returnsfalseat line 194, a cancel can occur before the status is patched to"completed"at line 213.🔧 Suggested guard
export const updateGenerationStatus = internalMutation({ @@ handler: async (ctx, args) => { + const current = await ctx.db.get(args.generationId) + if (!current) return + if (current.status === "cancelled" && args.status !== "cancelled") { + return + } + const updates: { status: typeof args.status updatedAt: number errorMessage?: string errorCode?: number imageId?: typeof args.imageId retryCount?: number } = { status: args.status, updatedAt: Date.now(), } @@ await ctx.db.patch(args.generationId, updates) }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/singleGeneration.ts` around lines 242 - 285, updateGenerationStatus currently patches status blindly which can overwrite a user-initiated "cancelled" state; before calling ctx.db.patch, fetch the current record (use ctx.db.get(args.generationId)) and if its status === "cancelled" and args.status !== "cancelled", return early (skip the patch); this preserves legitimate transitions to "cancelled" but prevents later processor updates from reverting a cancelled generation. Ensure you still apply other non-status fields if you decide to allow them, or skip entirely when skipping status changes.
🧹 Nitpick comments (2)
hooks/queries/use-generate-image.ts (1)
245-250: Remove theascast when assigningimage.params.You can eliminate this cast by typing
ActiveGenerationEntry.paramsasGeneratedImage["params"]directly, soparams: entry.paramsremains fully typed without assertions.As per coding guidelines, "Avoid
ascasts in TypeScript; prefersatisfies, generics, and runtime validation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/queries/use-generate-image.ts` around lines 245 - 250, The assignment uses an unnecessary type assertion for image.params; remove the `as` cast in the object literal and instead make the source type match the target by changing the type definition of ActiveGenerationEntry.params to GeneratedImage["params"] (or declare ActiveGenerationEntry with a generic so its params property is typed as GeneratedImage["params"]). Update the ActiveGenerationEntry type (where it’s defined) so entry.params already has the correct type, then simply assign `params: entry.params` in the GeneratedImage construction (ensure any dependent code compiles after the type adjustment).components/studio/layout/studio-shell.tsx (1)
272-276: Avoid cast-based ID conversion inhandleCancelSingleItem.The cast here can be removed by keeping queue item IDs strongly typed end-to-end (e.g., typed as the same ID type expected by
cancelGenerationById) instead of convertingstringat call time.As per coding guidelines, "Avoid
ascasts in TypeScript; prefersatisfies, generics, and runtime validation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/studio/layout/studio-shell.tsx` around lines 272 - 276, handleCancelSingleItem currently accepts id: string and calls cancelGenerationById with an unnecessary type cast; change the parameter type of handleCancelSingleItem to the exact ID type cancelGenerationById expects (or a shared alias/type) so the types flow end-to-end and remove the cast, and update any callers or the queue item type so they pass that strongly-typed ID instead of a string; reference handleCancelSingleItem and cancelGenerationById and ensure the queue item interface/generic that supplies the id uses the same ID type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/storage/r2-client.test.ts`:
- Around line 3-5: The hoisted mockSend created via vi.hoisted retains its
implementation across tests (you set mockSend.mockResolvedValue({}) at line
~153) and vi.clearAllMocks() only clears call history; fix by resetting the
hoisted mock implementation between tests: add mockSend.mockReset() (or
mockSend.mockRestore() if using spies) in a beforeEach/afterEach hook (next to
where vi.clearAllMocks() is called) so mockSend (from vi.hoisted) has no
persistent implementation leaking between tests.
- Around line 34-45: The test currently uses a double cast "as
Partial<crypto.Hash> as crypto.Hash" for mockHashInstance which bypasses
TypeScript safety; instead, instantiate a real crypto.Hash via crypto.createHash
(or call crypto.createHash("sha256") once), then spyOn or stub only the digest
(and update if needed) on that real Hash instance and have vi.spyOn(crypto,
"createHash").mockReturnValue(theRealHash) so createHash, mockHashInstance, and
digest maintain proper types without using "as" casts; update the mock setup to
replace the double assertion with this real-instance-and-spy approach for
createHash and digest.
---
Outside diff comments:
In `@convex/singleGeneration.ts`:
- Around line 242-285: updateGenerationStatus currently patches status blindly
which can overwrite a user-initiated "cancelled" state; before calling
ctx.db.patch, fetch the current record (use ctx.db.get(args.generationId)) and
if its status === "cancelled" and args.status !== "cancelled", return early
(skip the patch); this preserves legitimate transitions to "cancelled" but
prevents later processor updates from reverting a cancelled generation. Ensure
you still apply other non-status fields if you decide to allow them, or skip
entirely when skipping status changes.
---
Nitpick comments:
In `@components/studio/layout/studio-shell.tsx`:
- Around line 272-276: handleCancelSingleItem currently accepts id: string and
calls cancelGenerationById with an unnecessary type cast; change the parameter
type of handleCancelSingleItem to the exact ID type cancelGenerationById expects
(or a shared alias/type) so the types flow end-to-end and remove the cast, and
update any callers or the queue item type so they pass that strongly-typed ID
instead of a string; reference handleCancelSingleItem and cancelGenerationById
and ensure the queue item interface/generic that supplies the id uses the same
ID type.
In `@hooks/queries/use-generate-image.ts`:
- Around line 245-250: The assignment uses an unnecessary type assertion for
image.params; remove the `as` cast in the object literal and instead make the
source type match the target by changing the type definition of
ActiveGenerationEntry.params to GeneratedImage["params"] (or declare
ActiveGenerationEntry with a generic so its params property is typed as
GeneratedImage["params"]). Update the ActiveGenerationEntry type (where it’s
defined) so entry.params already has the correct type, then simply assign
`params: entry.params` in the GeneratedImage construction (ensure any dependent
code compiles after the type adjustment).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
components/studio/canvas/image-canvas.test.tsxcomponents/studio/controls/prompt-section.test.tsxcomponents/studio/controls/prompt-section.tsxcomponents/studio/layout/studio-shell.test.tsxcomponents/studio/layout/studio-shell.tsxconvex/lib/r2.tsconvex/singleGeneration.tsconvex/singleGenerationProcessor.tshooks/queries/use-generate-image.test.tsxhooks/queries/use-generate-image.tslib/storage/r2-client.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- components/studio/controls/prompt-section.tsx
- Created convex/admin.ts with grantProByEmail and revokeProByEmail internalMutations\n- Updated generated types
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
convex/admin.ts (1)
53-60: Inconsistent return type shapes between success paths.When the user already has an active subscription, the return object includes
alreadyActivebut omitssyntheticSubscriptionId. When granting new access, it includes both. Consider returning a consistent shape to simplify caller logic.♻️ Proposed fix for consistent return shape
if (alreadyActive) { return { success: true, message: `User ${args.email} (${clerkId}) already has an active Pro subscription.`, + syntheticSubscriptionId: null, alreadyActive: true, } }Also applies to: 98-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/admin.ts` around lines 53 - 60, The early return that checks existingSubs.some(...) returns { success, message, alreadyActive } while the other path returns { success, message, syntheticSubscriptionId, alreadyActive }, causing inconsistent shapes; update the early-return branch (the block using existingSubs and alreadyActive) to include syntheticSubscriptionId (set to null or the same id schema) and ensure all grant/new-access returns always include both syntheticSubscriptionId and alreadyActive so callers can rely on a consistent response shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/admin.ts`:
- Around line 24-26: The documentation mentions a dev-only guard that isn't
implemented; add a guard at the start of the exported handler in convex/admin.ts
that reads process.env.CONVEX_CLOUD_URL and the deployment name (e.g.,
process.env.CONVEX_DEPLOYMENT or the variable used to represent the deployment)
and if CONVEX_CLOUD_URL includes ".convex.cloud" and the deployment name does
NOT start with "dev:" throw an Error with a clear message (e.g., "Dev-only admin
handler blocked: production deployment detected") so the handler enforces the
documented behavior.
---
Nitpick comments:
In `@convex/admin.ts`:
- Around line 53-60: The early return that checks existingSubs.some(...) returns
{ success, message, alreadyActive } while the other path returns { success,
message, syntheticSubscriptionId, alreadyActive }, causing inconsistent shapes;
update the early-return branch (the block using existingSubs and alreadyActive)
to include syntheticSubscriptionId (set to null or the same id schema) and
ensure all grant/new-access returns always include both syntheticSubscriptionId
and alreadyActive so callers can rely on a consistent response shape.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (1)
convex/admin.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74cd2d83d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+129
to
+131
| if (await isCancelled()) { | ||
| console.log(`${logger} Generation ${args.generationId} was cancelled before provider request`) | ||
| return |
There was a problem hiding this comment.
This cancellation check runs only after the action has already patched the row to processing, so a cancel request that lands between the initial getGeneration() read and that patch can be overwritten. In that race, the record is no longer cancelled, isCancelled() returns false, and the job still calls the provider and persists output even though the user cancelled it. Make the pending→processing transition conditional (or re-read status immediately before patching) so a cancel cannot be lost in this window.
Useful? React with 👍 / 👎.
This was referenced Mar 4, 2026
Merged
This was referenced Mar 11, 2026
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.
$(cat <<'EOF'
Summary by CodeRabbit
New Features
Improvements
Bug Fixes