feat(chat): add image attachment support#5045
Merged
rschlaefli merged 26 commits intov3from Apr 16, 2026
Merged
Conversation
dosubot
bot
added
the
size:XL
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end image attachment support: client adapter and composer UI, message/store/type updates, API request/response and persistence (Prisma model + migration), runtime wiring to propagate image data through message conversions, and credit accounting for image description generation. Changes
Sequence DiagramsequenceDiagram
actor User
participant Composer as Composer UI
participant Adapter as Image Adapter
participant Store as Composer Store
participant ClientHook as useChatResponse
participant API as Chat API
participant DB as Database
participant Thread as Thread UI
User->>Composer: select image file
Composer->>Adapter: add(file)
Adapter->>Adapter: validate/convert HEIC, downscale if needed
Adapter-->>Composer: PendingAttachment (data:image/... base64)
Composer->>Store: set pending attachment (preview)
User->>Composer: submit message with attachment
Composer->>ClientHook: send (includes root imageBase64)
ClientHook->>API: POST /chat { messages..., imageBase64? }
API->>API: validate imageBase64, generate imageDescription
API->>DB: upsert ChatMessage and ChatAttachment (imageBase64, imageDescription)
DB-->>API: persisted record
API-->>ClientHook: response stream with message + attachment metadata
Thread->>API: GET /threads/.../messages
API->>DB: fetch messages including attachment
DB-->>API: messages + attachments
API-->>Thread: messages with attachment objects
Thread->>User: render message images and descriptions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
apps/chat/src/app/api/chatbots/[chatbotId]/chat/route.ts (1)
935-990:⚠️ Potential issue | 🟠 MajorMake the message and attachment write atomic.
The message create/update and
chatAttachment.upsert()are separate writes inside the sametry. If the attachment write fails after the message write succeeds, the request still completes but the attachment disappears on reload. Please persist these dependent changes in one transaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts around lines 935 - 990, Wrap the message create/update and attachment upsert in a single Prisma transaction so both succeed or both fail: replace the separate calls to prisma.chatMessage.updateMany / prisma.chatMessage.create and the upsertAttachment helper with a single prisma.$transaction block (or prisma.$transaction(async (tx) => { ... })) and call tx.chatMessage.updateMany / tx.chatMessage.create and tx.chatAttachment.upsert inside it (or refactor upsertAttachment to accept the transaction client and use tx.chatAttachment.upsert); ensure you use the same IDs (userMessageId or lastMessage.id) inside the transaction and handle the conditional create vs update logic within that transaction so the message and its attachment are persisted atomically.apps/chat/src/components/thread.tsx (1)
523-563:⚠️ Potential issue | 🟠 MajorEdits can't remove or replace an attached image.
The main composer exposes add/remove attachment controls, but
EditComposeronly shows a preview ofimageBase64. Once a user sends the wrong image, editing the message keeps reusing that attachment with no way to clear or replace it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/chat/src/components/thread.tsx` around lines 523 - 563, The preview of imageBase64 inside ComposerPrimitive.Root has no control to clear or replace the attachment, so EditComposer re-sends the old image; add a visible remove/replace control (e.g., a small "Remove" or "Replace" button/icon next to the <img> preview) that clears the local imageBase64 state and invokes the existing edit callbacks (e.g., call a passed-in onClearAttachment or onUpdateAttachment prop) so ComposerPrimitive.Send uses the updated state when submitting; update EditComposer to accept and wire those handlers and ensure ComposerPrimitive.Input or an upload control can set a new imageBase64 when replacing.
🧹 Nitpick comments (4)
apps/chat/src/app/RuntimeProvider.tsx (1)
18-18: Consider using path alias for import consistency.The import uses a relative path (
../lib/attachments/imageAttachmentAdapter) while other imports in this file usesrc/prefix paths (lines 10-16). As per coding guidelines, prefer@or~path aliases for imports.♻️ Suggested change
-import { imageAttachmentAdapter } from '../lib/attachments/imageAttachmentAdapter' +import { imageAttachmentAdapter } from 'src/lib/attachments/imageAttachmentAdapter'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/chat/src/app/RuntimeProvider.tsx` at line 18, Replace the relative import of imageAttachmentAdapter with the same path-alias style used by other imports in this file: update the import of imageAttachmentAdapter (symbol: imageAttachmentAdapter) to use the project's alias-based path (e.g., the same 'src/...' prefix used on lines 10-16) so imports are consistent; verify the alias exists in tsconfig/webpack and adjust the import string accordingly.apps/chat/src/lib/attachments/imageAttachmentAdapter.ts (1)
9-35: LGTM with optional enhancement.The adapter correctly implements the
AttachmentAdapterinterface with appropriate size validation and error messaging. The use ofsatisfies PendingAttachmentensures type safety while preserving literal types.Consider adding explicit file type validation in
add(), since theacceptproperty only hints to the file picker but doesn't prevent programmatic additions of unsupported file types.♻️ Optional file type validation
async add({ file }) { + const allowedTypes = ['image/jpeg', 'image/png', 'image/gif', 'image/webp'] + if (!allowedTypes.includes(file.type)) { + throw new Error( + `Unsupported image type: ${file.type}. Allowed types: ${allowedTypes.join(', ')}` + ) + } + if (file.size > MAX_IMAGE_SIZE_BYTES) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/chat/src/lib/attachments/imageAttachmentAdapter.ts` around lines 9 - 35, The add() method in imageAttachmentAdapter currently only enforces size limits but not MIME type validation, so add an explicit check inside imageAttachmentAdapter.add that verifies file.type against the allowed MIME types (use the existing accept value or a small array of allowed types) before reading the file; if the type is not allowed, throw a descriptive Error (similar to the size check). Keep the error messaging consistent with the size error, reference file.name/file.type in the message, and preserve the returned PendingAttachment shape and use of crypto.randomUUID().packages/prisma/src/prisma/schema/migrations/20260317133854_add_chat_attachment/migration.sql (1)
17-21: Remove redundant index onmessageId.The schema declares both
@uniqueon themessageIdfield and@@index([messageId]), resulting in duplicate indexes in the migration (lines 18 and 21). A unique constraint already provides index functionality for efficient lookups, making the additional index redundant and wasteful of storage.Remove
@@index([messageId])from the ChatAttachment model in the schema.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/prisma/src/prisma/schema/migrations/20260317133854_add_chat_attachment/migration.sql` around lines 17 - 21, The ChatAttachment model has a redundant non-unique index on messageId producing duplicate indexes in the migration: remove the explicit @@index([messageId]) from the ChatAttachment model so only the `@unique` on messageId remains (which already creates the necessary unique index), then regenerate the migration so the duplicate index ("ChatAttachment_messageId_idx") is not created; look for references to messageId, the ChatAttachment model, @@index and `@unique` when making the change.apps/chat/src/components/thread.tsx (1)
280-370: Usefunctiondeclarations for the new composer components.
ComposerAttachments,ComposerImageAttachment, andComposerAttachButtonwere added in acomponents/**file, so they should follow the local component declaration convention instead ofconst ...: FC =.As per coding guidelines,
**/{components,app}/**/*.{ts,tsx}: Component files should use PascalCase naming withfunctionkeyword for component declarations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/chat/src/components/thread.tsx` around lines 280 - 370, Replace the three arrow-function component declarations with function declarations to follow the project's component convention: change "const ComposerAttachments: FC = () => { ... }" to "function ComposerAttachments() { ... }", change "const ComposerImageAttachment: FC = () => { ... }" to "function ComposerImageAttachment() { ... }", and change "const ComposerAttachButton: FC = () => { ... }" to "function ComposerAttachButton() { ... }"; keep all internal logic, hooks (useChatUi, useThreadComposerAttachment), props (none) and JSX unchanged, and remove the FC type annotations so the components match the PascalCase function-style component pattern used in components/** files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts:
- Around line 822-842: The generateText(...) call that creates imageDescription
is not being accounted for in usage/cost tracking; update the flow so that the
result from generateText (function generateText) is fed into the same cost
accounting path as streamText: calculate its cost via calcCost(...) and call
CreditsService.decrementCredits(...) with the computed cost (or aggregate it
with the later streamText cost) before proceeding, ensuring imageDescription
generation consumes credits and is included in usage reporting.
- Around line 591-597: The current validation for imageBase64
(z.string().max(...).refine(...).optional()) is too permissive; update the
refine to only accept data URLs for the exact MIME types we support (jpeg, png,
gif, webp) and require the ';base64,' marker and a base64 payload. In the
imageBase64 validator in route.ts, replace the startsWith check with a strict
regex/refine that enforces
/^data:image\/(jpeg|png|gif|webp);base64,[A-Za-z0-9+/=]+$/ (or equivalent logic)
so SVG and non-base64 payloads are rejected while keeping the same max length
and optional flag.
---
Outside diff comments:
In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts:
- Around line 935-990: Wrap the message create/update and attachment upsert in a
single Prisma transaction so both succeed or both fail: replace the separate
calls to prisma.chatMessage.updateMany / prisma.chatMessage.create and the
upsertAttachment helper with a single prisma.$transaction block (or
prisma.$transaction(async (tx) => { ... })) and call tx.chatMessage.updateMany /
tx.chatMessage.create and tx.chatAttachment.upsert inside it (or refactor
upsertAttachment to accept the transaction client and use
tx.chatAttachment.upsert); ensure you use the same IDs (userMessageId or
lastMessage.id) inside the transaction and handle the conditional create vs
update logic within that transaction so the message and its attachment are
persisted atomically.
In `@apps/chat/src/components/thread.tsx`:
- Around line 523-563: The preview of imageBase64 inside ComposerPrimitive.Root
has no control to clear or replace the attachment, so EditComposer re-sends the
old image; add a visible remove/replace control (e.g., a small "Remove" or
"Replace" button/icon next to the <img> preview) that clears the local
imageBase64 state and invokes the existing edit callbacks (e.g., call a
passed-in onClearAttachment or onUpdateAttachment prop) so
ComposerPrimitive.Send uses the updated state when submitting; update
EditComposer to accept and wire those handlers and ensure
ComposerPrimitive.Input or an upload control can set a new imageBase64 when
replacing.
---
Nitpick comments:
In `@apps/chat/src/app/RuntimeProvider.tsx`:
- Line 18: Replace the relative import of imageAttachmentAdapter with the same
path-alias style used by other imports in this file: update the import of
imageAttachmentAdapter (symbol: imageAttachmentAdapter) to use the project's
alias-based path (e.g., the same 'src/...' prefix used on lines 10-16) so
imports are consistent; verify the alias exists in tsconfig/webpack and adjust
the import string accordingly.
In `@apps/chat/src/components/thread.tsx`:
- Around line 280-370: Replace the three arrow-function component declarations
with function declarations to follow the project's component convention: change
"const ComposerAttachments: FC = () => { ... }" to "function
ComposerAttachments() { ... }", change "const ComposerImageAttachment: FC = ()
=> { ... }" to "function ComposerImageAttachment() { ... }", and change "const
ComposerAttachButton: FC = () => { ... }" to "function ComposerAttachButton() {
... }"; keep all internal logic, hooks (useChatUi, useThreadComposerAttachment),
props (none) and JSX unchanged, and remove the FC type annotations so the
components match the PascalCase function-style component pattern used in
components/** files.
In `@apps/chat/src/lib/attachments/imageAttachmentAdapter.ts`:
- Around line 9-35: The add() method in imageAttachmentAdapter currently only
enforces size limits but not MIME type validation, so add an explicit check
inside imageAttachmentAdapter.add that verifies file.type against the allowed
MIME types (use the existing accept value or a small array of allowed types)
before reading the file; if the type is not allowed, throw a descriptive Error
(similar to the size check). Keep the error messaging consistent with the size
error, reference file.name/file.type in the message, and preserve the returned
PendingAttachment shape and use of crypto.randomUUID().
In
`@packages/prisma/src/prisma/schema/migrations/20260317133854_add_chat_attachment/migration.sql`:
- Around line 17-21: The ChatAttachment model has a redundant non-unique index
on messageId producing duplicate indexes in the migration: remove the explicit
@@index([messageId]) from the ChatAttachment model so only the `@unique` on
messageId remains (which already creates the necessary unique index), then
regenerate the migration so the duplicate index ("ChatAttachment_messageId_idx")
is not created; look for references to messageId, the ChatAttachment model,
@@index and `@unique` when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91204053-cbe9-4b01-9856-b98ad2830508
📒 Files selected for processing (11)
apps/chat/src/app/RuntimeProvider.tsxapps/chat/src/app/api/chatbots/[chatbotId]/chat/route.tsapps/chat/src/app/api/chatbots/[chatbotId]/threads/[threadId]/messages/route.tsapps/chat/src/components/thread.tsxapps/chat/src/hooks/useChatResponse.tsapps/chat/src/hooks/useThreadManagement.tsapps/chat/src/lib/api/types.tsapps/chat/src/lib/attachments/imageAttachmentAdapter.tsapps/chat/src/stores/chatStore.tspackages/prisma/src/prisma/schema/chat.prismapackages/prisma/src/prisma/schema/migrations/20260317133854_add_chat_attachment/migration.sql
…ot_add_image_attachments
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/chat/src/app/api/chatbots/[chatbotId]/chat/route.ts (2)
981-991: Use optional chaining for cleaner guard clause.The condition can be simplified with optional chaining as suggested by static analysis.
♻️ Suggested refactor
for (let i = 0; i < messages.length; i++) { const msg = messages[i] - if (!msg || msg.role !== 'user' || !msg.imageDescription) continue + if (msg?.role !== 'user' || !msg?.imageDescription) continue // skip the current message — already handled by the describe-once block above if (imageDescription && msg.id === lastMessage?.id) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts around lines 981 - 991, Replace the verbose null checks in the loop with optional chaining: instead of checking "!msg || msg.role !== 'user' || !msg.imageDescription" use a single guard that leverages msg?.role and msg?.imageDescription (e.g., if (msg?.role !== 'user' || !msg?.imageDescription) continue) so the loop over messages, the skip-for-current-message check (comparing imageDescription && msg.id === lastMessage?.id), and the modelMessages[i] augmentation (spreading existing content and appending the attached image description) remain unchanged; update the guard in the for loop accordingly to make the code cleaner and safer.
901-904: Refactor nested template literals for readability.The prompt construction uses nested template literals which reduces clarity.
♻️ Suggested refactor
{ type: 'text', - text: `${lastMessage?.content ? `User message context: ${lastMessage.content}\n\n` : ''}Describe this image in detail. Include all visible text, diagrams, charts, equations, labels, and notable visual elements. This description will serve as context for an ongoing conversation.`, + text: [ + lastMessage?.content + ? `User message context: ${lastMessage.content}` + : '', + 'Describe this image in detail. Include all visible text, diagrams, charts, equations, labels, and notable visual elements. This description will serve as context for an ongoing conversation.', + ] + .filter(Boolean) + .join('\n\n'), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts around lines 901 - 904, The prompt's nested template literal (in the object with type: 'text' where text currently uses `${lastMessage?.content ? `User message context: ${lastMessage.content}\n\n` : ''}Describe this image...`) should be simplified for readability: extract the optional prefix into a local variable (e.g., userContext or prefix) that computes lastMessage?.content ? `User message context: ${lastMessage.content}\n\n` : '' and then set the text field by concatenating or a single template `${userContext}Describe this image in detail...`, keeping the rest of the prompt unchanged; update the object where text is defined to use that variable instead of the nested template literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts:
- Around line 981-991: Replace the verbose null checks in the loop with optional
chaining: instead of checking "!msg || msg.role !== 'user' ||
!msg.imageDescription" use a single guard that leverages msg?.role and
msg?.imageDescription (e.g., if (msg?.role !== 'user' || !msg?.imageDescription)
continue) so the loop over messages, the skip-for-current-message check
(comparing imageDescription && msg.id === lastMessage?.id), and the
modelMessages[i] augmentation (spreading existing content and appending the
attached image description) remain unchanged; update the guard in the for loop
accordingly to make the code cleaner and safer.
- Around line 901-904: The prompt's nested template literal (in the object with
type: 'text' where text currently uses `${lastMessage?.content ? `User message
context: ${lastMessage.content}\n\n` : ''}Describe this image...`) should be
simplified for readability: extract the optional prefix into a local variable
(e.g., userContext or prefix) that computes lastMessage?.content ? `User message
context: ${lastMessage.content}\n\n` : '' and then set the text field by
concatenating or a single template `${userContext}Describe this image in
detail...`, keeping the rest of the prompt unchanged; update the object where
text is defined to use that variable instead of the nested template literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62ed2f64-3d80-43c4-9dda-781ba598e2c8
📒 Files selected for processing (1)
apps/chat/src/app/api/chatbots/[chatbotId]/chat/route.ts
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts:
- Around line 1040-1042: upsertAttachment currently returns early when both
imageBase64 and imageDescription are empty, leaving any previous chatAttachment
orphaned; update upsertAttachment to first check the DB for an existing
attachment for the given messageId and, if no new image data is provided, delete
that existing chatAttachment (and update any related flags on chatMessage, e.g.,
hasAttachment) before returning; otherwise proceed with the existing upsert
logic to create/update the attachment. Apply the same delete-if-empty logic to
the other similar upsertAttachment occurrence referenced in the review.
- Around line 899-916: The image captioning branch should only run when the most
recent transcript message is from the user: guard uses of imageBase64 by
checking lastMessage?.role === 'user' before calling generateText (and before
any other image-related calls), rejecting/ignoring imageBase64 otherwise to
avoid unnecessary captioning/charges; update the blocks that call generateText
(referencing imageBase64 and generateText) and the other similar branches you
noted (around the imageBase64 usages at the other locations) to early-return or
clear imageBase64 when lastMessage?.role !== 'user'.
- Around line 919-943: The code currently decrements imageDescriptionCost
immediately inside the captioning branch (CreditsService.decrementCredits with
participantId, chatbotId, imageDescriptionCost) and then also folds
imageDescriptionCost into creditsUsed which is decremented again on
finish/abort, causing double billing; remove the immediate decrement calls in
the captioning/image-description branches (the places that compute
descriptionResult and set imageDescriptionCost) and instead only add
imageDescriptionCost to the creditsUsed accumulator (the creditsUsed variable)
so the single CreditsService.decrementCredits call at the unified finish/abort
path handles billing once; ensure any try/catch around
CreditsService.decrementCredits is preserved only where the final ledger
decrement happens.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5d2be13-b4c3-4446-ac4e-edad52340186
📒 Files selected for processing (1)
apps/chat/src/app/api/chatbots/[chatbotId]/chat/route.ts
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
chatbot_add_image_attachments
|
| Run status |
|
| Run duration | 09m 18s |
| Commit |
|
| Committer | Jannis Alsbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
758
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/chat/src/app/api/chatbots/[chatbotId]/chat/route.ts (3)
1062-1064:⚠️ Potential issue | 🟡 MinorDelete stale attachments when a resend no longer carries image data.
The early return here preserves any existing
chatAttachmentrow for thatmessageId, so editing or resending a message without an image keeps the old preview/description alive.Suggested fix
const upsertAttachment = async (messageId: string) => { - if (!imageBase64 && !imageDescription) return + if (!imageBase64 && !imageDescription) { + await prisma.chatAttachment.deleteMany({ + where: { messageId }, + }) + return + } await prisma.chatAttachment.upsert({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts around lines 1062 - 1064, The upsertAttachment function currently returns early if both imageBase64 and imageDescription are falsy, which leaves any existing chatAttachment row for that messageId intact; instead, when imageBase64 and imageDescription are both empty you should delete the stale attachment for that messageId. Update upsertAttachment to, when !imageBase64 && !imageDescription, call the code path that removes the existing chatAttachment (e.g., delete or deleteMany on chatAttachment filtered by messageId) before returning; otherwise proceed with the existing create/update logic to upsert the attachment. Ensure you reference upsertAttachment, messageId, imageBase64, imageDescription and the chatAttachment delete operation in your change.
898-915:⚠️ Potential issue | 🟠 MajorReject
imageBase64unless the latest message is from the user.This still calls
generateText()for any request withimageBase64. If the transcript ends with an assistant message, the server can caption and bill an image that never gets attached or persisted by the later user-only paths.Suggested fix
- if (imageBase64) { + if (imageBase64) { + if (lastMessage?.role !== 'user') { + return NextResponse.json( + { + error: + 'Image attachments are only supported on the latest user message', + }, + { status: 400 } + ) + } + try { const descriptionResult = await generateText({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts around lines 898 - 915, The current flow calls generateText whenever imageBase64 is present, even if the transcript's lastMessage is from the assistant; update the logic so image handling only proceeds when the latest message is from the user (check lastMessage?.role === 'user' or equivalent) — if the last message is not from the user then reject/ignore imageBase64 (e.g., early return or skip the generateText/image-caption branch) to prevent captioning/billing images that aren't tied to a user message; adjust the conditional around the generateText call and any downstream use of the generated description to reference imageBase64 only when lastMessage?.role === 'user'.
918-941:⚠️ Potential issue | 🟠 MajorDon't decrement
imageDescriptionCostinside the captioning branch.
imageDescriptionCostis already folded intocreditsUsedlater, so this immediate decrement can bill successful or aborted image requests twice. Keep the cost inimageDescriptionCostand let the unified finish/abort path charge once.Suggested fix
- // deduct credits for image description generation if (descriptionResult.usage) { imageDescriptionCost = calcCost( selectedModelConfig.cost, descriptionResult.usage.inputTokens || 0, descriptionResult.usage.outputTokens || 0 ) - if (imageDescriptionCost > 0) { - try { - await CreditsService.decrementCredits( - participantId, - chatbotId, - imageDescriptionCost - ) - } catch (creditsError) { - console.error( - 'Failed to decrement credits for image description:', - { - requestId, - error: creditsError, - } - ) - } - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts around lines 918 - 941, The code currently decrements credits inside the image captioning branch by calling CreditsService.decrementCredits(participantId, chatbotId, imageDescriptionCost) which causes double-billing because imageDescriptionCost is also added into creditsUsed and charged in the unified finish/abort path; remove the try/catch block and the call to CreditsService.decrementCredits in the block that handles descriptionResult.usage (the section computing imageDescriptionCost) and leave imageDescriptionCost assigned so the later unified billing logic charges it exactly once.
🧹 Nitpick comments (2)
apps/chat/src/components/thread.tsx (2)
26-26: Use the repo alias for the new composer store import.Line 26 introduces another relative import in a file type where the project standard is
@/~aliases.As per coding guidelines, `**/*.{ts,tsx,js,jsx}`: Use `@` and `~` path aliases for imports instead of relative paths.Suggested fix
-import { useComposerStore } from '../stores/composerStore' +import { useComposerStore } from '@/src/stores/composerStore'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/chat/src/components/thread.tsx` at line 26, Replace the relative import of useComposerStore with the project path alias (use @ or ~) to follow the repo import convention; update the import that currently reads import { useComposerStore } from '../stores/composerStore' so it imports from the aliased module (e.g. '@/stores/composerStore' or '~/stores/composerStore') and ensure any IDE/tsconfig paths are already configured so useComposerStore resolves correctly in thread.tsx.
300-390: Switch the newly added components to function declarations.
ComposerAttachments,ComposerImageAttachment, andComposerAttachButtonare all new in this hunk, but they're still declared asconst ...: FC =. This directory's convention isfunction ComponentName().As per coding guidelines, `**/{components,app}/**/*.{ts,tsx}`: Component files should use PascalCase naming with `function` keyword for component declarations.Example refactor
-const ComposerAttachments: FC = () => { +function ComposerAttachments() { return ( <div className="flex w-full flex-wrap gap-2 py-2 empty:hidden"> <ComposerPrimitive.Attachments components={{ Image: ComposerImageAttachment, @@ /> </div> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/chat/src/components/thread.tsx` around lines 300 - 390, Replace the three arrow-const component declarations with function declarations to follow project convention: change "const ComposerAttachments: FC = () => { ... }", "const ComposerImageAttachment: FC = () => { ... }", and "const ComposerAttachButton: FC = () => { ... }" into "function ComposerAttachments() { ... }", "function ComposerImageAttachment() { ... }", and "function ComposerAttachButton() { ... }" respectively, keeping the bodies, hooks (useChatUi, useThreadComposerAttachment), JSX, and any local variables unchanged; remove the explicit ": FC" typing (no props change needed) so the components remain named exactly the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts:
- Around line 1012-1029: The Prisma lookup for prior image descriptions
(prisma.chatAttachment.findMany) only filters by messageId and can return
attachments from other threads; narrow the query to the current thread by also
filtering on the owning thread id (use the already-fetched owningThread
identifier) and ensure owningThread is present before running the query so only
attachments with messageId IN priorMessageIds AND threadId === owningThread.id
are returned; update the findMany call that builds descriptionsByMsgId to
include this thread-scoped where clause.
---
Duplicate comments:
In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts:
- Around line 1062-1064: The upsertAttachment function currently returns early
if both imageBase64 and imageDescription are falsy, which leaves any existing
chatAttachment row for that messageId intact; instead, when imageBase64 and
imageDescription are both empty you should delete the stale attachment for that
messageId. Update upsertAttachment to, when !imageBase64 && !imageDescription,
call the code path that removes the existing chatAttachment (e.g., delete or
deleteMany on chatAttachment filtered by messageId) before returning; otherwise
proceed with the existing create/update logic to upsert the attachment. Ensure
you reference upsertAttachment, messageId, imageBase64, imageDescription and the
chatAttachment delete operation in your change.
- Around line 898-915: The current flow calls generateText whenever imageBase64
is present, even if the transcript's lastMessage is from the assistant; update
the logic so image handling only proceeds when the latest message is from the
user (check lastMessage?.role === 'user' or equivalent) — if the last message is
not from the user then reject/ignore imageBase64 (e.g., early return or skip the
generateText/image-caption branch) to prevent captioning/billing images that
aren't tied to a user message; adjust the conditional around the generateText
call and any downstream use of the generated description to reference
imageBase64 only when lastMessage?.role === 'user'.
- Around line 918-941: The code currently decrements credits inside the image
captioning branch by calling CreditsService.decrementCredits(participantId,
chatbotId, imageDescriptionCost) which causes double-billing because
imageDescriptionCost is also added into creditsUsed and charged in the unified
finish/abort path; remove the try/catch block and the call to
CreditsService.decrementCredits in the block that handles
descriptionResult.usage (the section computing imageDescriptionCost) and leave
imageDescriptionCost assigned so the later unified billing logic charges it
exactly once.
---
Nitpick comments:
In `@apps/chat/src/components/thread.tsx`:
- Line 26: Replace the relative import of useComposerStore with the project path
alias (use @ or ~) to follow the repo import convention; update the import that
currently reads import { useComposerStore } from '../stores/composerStore' so it
imports from the aliased module (e.g. '@/stores/composerStore' or
'~/stores/composerStore') and ensure any IDE/tsconfig paths are already
configured so useComposerStore resolves correctly in thread.tsx.
- Around line 300-390: Replace the three arrow-const component declarations with
function declarations to follow project convention: change "const
ComposerAttachments: FC = () => { ... }", "const ComposerImageAttachment: FC =
() => { ... }", and "const ComposerAttachButton: FC = () => { ... }" into
"function ComposerAttachments() { ... }", "function ComposerImageAttachment() {
... }", and "function ComposerAttachButton() { ... }" respectively, keeping the
bodies, hooks (useChatUi, useThreadComposerAttachment), JSX, and any local
variables unchanged; remove the explicit ": FC" typing (no props change needed)
so the components remain named exactly the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45dfc268-d01d-414b-9b1a-6cbb2849c423
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/chat/package.jsonapps/chat/src/app/api/chatbots/[chatbotId]/chat/route.tsapps/chat/src/components/thread.tsxapps/chat/src/hooks/useChatResponse.tsapps/chat/src/lib/attachments/imageAttachmentAdapter.tsapps/chat/src/stores/composerStore.ts
✅ Files skipped from review due to trivial changes (2)
- apps/chat/package.json
- apps/chat/src/hooks/useChatResponse.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/chat/src/lib/attachments/imageAttachmentAdapter.ts
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/chat/src/app/api/chatbots/[chatbotId]/chat/route.ts (1)
906-911: Extract nested template literal for readability.The nested template literal makes the prompt harder to read. Consider extracting the context prefix.
♻️ Suggested refactor
+ const contextPrefix = lastMessage?.content + ? `User message context: ${lastMessage.content}\n\n` + : '' content: [ { type: 'image', image: imageBase64 }, { type: 'text', - text: `${lastMessage?.content ? `User message context: ${lastMessage.content}\n\n` : ''}Describe this image in detail. Include all visible text, diagrams, charts, equations, labels, and notable visual elements. This description will serve as context for an ongoing conversation.`, + text: `${contextPrefix}Describe this image in detail. Include all visible text, diagrams, charts, equations, labels, and notable visual elements. This description will serve as context for an ongoing conversation.`, }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts around lines 906 - 911, The nested template literal inside the text message makes the prompt hard to read; extract the context prefix into a separate variable (e.g., const contextPrefix = lastMessage?.content ? `User message context: ${lastMessage.content}\n\n` : '') and then use that variable when building the text message in the messages array (the object with type: 'text' in route.ts), keeping the rest of the prompt unchanged; reference lastMessage and imageBase64 to locate the surrounding code and replace the inline `${lastMessage?.content ? ...}` expression with the new contextPrefix for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/chat/src/app/api/chatbots/`[chatbotId]/chat/route.ts:
- Around line 906-911: The nested template literal inside the text message makes
the prompt hard to read; extract the context prefix into a separate variable
(e.g., const contextPrefix = lastMessage?.content ? `User message context:
${lastMessage.content}\n\n` : '') and then use that variable when building the
text message in the messages array (the object with type: 'text' in route.ts),
keeping the rest of the prompt unchanged; reference lastMessage and imageBase64
to locate the surrounding code and replace the inline `${lastMessage?.content ?
...}` expression with the new contextPrefix for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 248e2fa4-83c6-4a29-a185-8750fea25bb0
📒 Files selected for processing (1)
apps/chat/src/app/api/chatbots/[chatbotId]/chat/route.ts
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
rschlaefli
requested review from
jabbadizzleCode
and removed request for
rschlaefli
jabbadizzleCode
approved these changes
Apr 14, 2026
Collaborator
jabbadizzleCode
left a comment
jabbadizzleCode
left a comment
There was a problem hiding this comment.
Looks good to me :). Before merging, maybe consider using Azure Blob Storage for the images instead of PostgreSQL (see comment).
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
…h-bf/klicker-uzh into chatbot_add_image_attachments
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
…ot_add_image_attachments
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
dosubot
bot
added
size:XXL
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
rschlaefli
changed the title
rschlaefli
changed the title
…ot_add_image_attachments # Conflicts: # AGENTS.md
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
v3
|
| Run status |
|
| Run duration | 09m 22s |
| Commit |
|
| Committer | Jannis Alsbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
1
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
757
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/e2e/U-catalog-workflow.cy.ts • 1 failed test
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sharpso the/chatrequest only carries full image bytes and avoids the local dev parse failure we hit during verificationVerification
pnpm --filter @klicker-uzh/chat checkpnpm --filter @klicker-uzh/chat test:runNotes
project/CHAT_IMAGE_IMPROVEMENTS.md