Baharsah/webp stiker ignoring conversion #451
Conversation
WalkthroughAdds sticker sending (including GIF→WebP conversion with gif2webp/ffmpeg/ImageMagick fallbacks), integrates media metadata persistence for sent messages, updates chat storage API usage and SQLite upsert/logging, and installs ImageMagick in the Docker runtime image. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SendSvc as serviceSend
participant Converter
participant Uploader as whatsmeow/upload
participant Storage as ChatStorage
Client->>SendSvc: SendSticker(request)
activate SendSvc
SendSvc->>SendSvc: getStickerPath(request) → path, cleanup
SendSvc->>SendSvc: processSticker(path) → processedMedia (mimetype, dims, isAnimated, data, thumb)
alt GIF requires conversion
SendSvc->>Converter: convertGIFToAnimatedWebP / convertGIFToWebPStatic
activate Converter
Converter->>Converter: try gif2webp
alt fails
Converter->>Converter: try ffmpeg / ImageMagick
end
Converter-->>SendSvc: WebP bytes
deactivate Converter
end
SendSvc->>Uploader: uploadMedia(mediaType, webpBytes)
activate Uploader
Uploader-->>SendSvc: UploadResponse (id, url, mediaKey, file hashes, size)
deactivate Uploader
SendSvc->>SendSvc: buildStickerMessage(request, uploaded, processedMedia)
SendSvc->>SendSvc: wrapAndStoreMessage(..., mediaInfo)
SendSvc->>Storage: StoreMessage(domainChatStorage.Message with mediaInfo)
activate Storage
Storage-->>SendSvc: ok / err
deactivate Storage
SendSvc-->>Client: response
deactivate SendSvc
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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 |
![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
🧹 Nitpick comments (3)
src/usecase/send.go (3)
80-108: Consider consolidating withwrapSendMessageto reduce duplication.This function is nearly identical to
wrapSendMessage(lines 49-78), differing only in the storage call. Consider refactoring to a single function that accepts an optionalmediaInfoparameter, which would improve maintainability and reduce code duplication.Example approach:
-func (service serviceSend) wrapSendMessage(ctx context.Context, recipient types.JID, msg *waE2E.Message, content string) (whatsmeow.SendResponse, error) { +func (service serviceSend) wrapSendMessage(ctx context.Context, recipient types.JID, msg *waE2E.Message, content string, mediaInfo *domainChatStorage.MediaInfo) (whatsmeow.SendResponse, error) { ts, err := whatsapp.GetClient().SendMessage(ctx, recipient, msg) if err != nil { return whatsmeow.SendResponse{}, err } senderJID := "" if whatsapp.GetClient().Store.ID != nil { senderJID = whatsapp.GetClient().Store.ID.String() } go func() { storeCtx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() - if err := service.chatStorageRepo.StoreSentMessageWithContext(storeCtx, ts.ID, senderJID, recipient.String(), content, ts.Timestamp); err != nil { + var err error + if mediaInfo != nil { + err = service.storeSentStickerWithMediaInfo(storeCtx, ts.ID, senderJID, recipient.String(), content, ts.Timestamp, mediaInfo) + } else { + err = service.chatStorageRepo.StoreSentMessageWithContext(storeCtx, ts.ID, senderJID, recipient.String(), content, ts.Timestamp) + } + if err != nil { if errors.Is(err, context.DeadlineExceeded) { logrus.Warn("Timeout storing sent message") } else { logrus.Warnf("Failed to store sent message: %v", err) } } }() return ts, nil }
1162-1170: Remove unnecessary temp file write for direct WebP inputs.When the input is already WebP (line 1158), the code saves it to a temporary file "for consistency with GIF flow" (line 1162), but this appears unnecessary since
stickerBytesalready contains the data. This adds I/O overhead without benefit.Consider removing the temporary file write:
if inputMimeType == "image/webp" { logrus.Info("Input is already WebP format, using directly") stickerBytes = inputFileBytes - // Save the WebP to a temporary file for consistency with GIF flow - webpTempPath := filepath.Join(absBaseDir, fmt.Sprintf("direct_webp_%s.webp", fiberUtils.UUIDv4())) - err = os.WriteFile(webpTempPath, stickerBytes, 0644) - if err != nil { - logrus.Warnf("Failed to save WebP temporarily: %v", err) - } else { - deletedItems = append(deletedItems, webpTempPath) - logrus.Infof("Saved direct WebP to: %s", webpTempPath) - } // For WebP files, try to get dimensions using the imaging library
1223-1231: Consider whether saving converted WebP to temp file is necessary.Similar to the direct WebP handling, this code saves the converted WebP bytes to a temporary file (lines 1223-1231), but
stickerBytesalready contains the data. If this is solely for debugging purposes, consider making it conditional on a debug flag to avoid unnecessary I/O in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
docker/golang.Dockerfile(1 hunks)src/usecase/send.go(8 hunks)
🔇 Additional comments (9)
docker/golang.Dockerfile (1)
18-18: LGTM: ImageMagick added for sticker conversion support.The addition of
imagemagickenables theconvertcommand used as a fallback in the GIF-to-WebP conversion workflow. This aligns with the new sticker functionality implemented insrc/usecase/send.go.src/usecase/send.go (8)
8-8: LGTM: Imports added for GIF and WebP handling.The new imports support the sticker conversion functionality:
image/gifenables GIF decoding for frame extractiongithub.com/chai2010/webpprovides Go-based WebP encoding as a fallbackAlso applies to: 26-26
110-154: LGTM: Media info storage implementation is solid.The function properly validates the JID, constructs a complete message with media metadata, and includes comprehensive logging for debugging. The direct use of
StoreMessageis appropriate for preserving complete media information.
275-275: LGTM: Comment clarifies WebP handling change.The updated comment accurately reflects that WebP images are now sent without conversion, which aligns with the PR's improved media handling strategy.
1044-1075: LGTM: Static WebP conversion provides a reliable fallback.This pure-Go implementation using the
chai2010/webplibrary is a solid fallback when external conversion tools are unavailable. The quality setting of 85 is appropriate for stickers, and error handling is comprehensive.
1284-1285: Derive conversion context from parent context.The conversion timeout context is created from the parent
ctx(line 1284), which is good. However, ensure this doesn't interfere with any existing deadline on the parent context. If the parent context has a shorter deadline, the conversion will respect it, which is the desired behavior.
1314-1347: LGTM: WebP validation and dimension adjustment logic is solid.The code properly validates the WebP output, attempts to extract dimensions with a reasonable fallback, and ensures dimensions comply with WhatsApp's 512x512 sticker limit while maintaining aspect ratio.
1358-1398: LGTM: Thumbnail generation and message construction are well-implemented.The thumbnail creation provides a smaller preview for the sender's device, and errors are handled gracefully as warnings. The sticker message includes all necessary metadata (dimensions, animation flag, media keys) for proper WhatsApp sticker delivery.
1416-1440: LGTM: Media info persistence and message sending are correctly implemented.The
MediaInfostruct is properly populated with all upload metadata, and the call towrapSendStickerMessageensures the sticker is sent and persisted with complete media information. The logging provides good visibility into the send operation.
|
Wait for pushing new solution asap |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/infrastructure/chatstorage/sqlite_repository.go (1)
166-220: Stop logging full message payloads at info level.These new INFO logs dump
message.Content(and the skip warning logs echo the same). That pushes raw user chat data/PII into persistent logs, which is a significant compliance/privacy risk in production observability systems. Please drop the plaintext content (or at least downgrade it to a redacted DEBUG trace) before merging.- logrus.Infof("🗄️ [DB] StoreMessage called - ID: %s, ChatJID: %s, Content: %s, MediaType: %s", - message.ID, message.ChatJID, message.Content, message.MediaType) + logrus.Infof("🗄️ [DB] StoreMessage called - ID: %s, ChatJID: %s, MediaType: %s", + message.ID, message.ChatJID, message.MediaType) … - logrus.Warnf("⚠️ [DB] Skipping empty message - ID: %s, Content: '%s', MediaType: '%s'", - message.ID, message.Content, message.MediaType) + logrus.Warnf("⚠️ [DB] Skipping empty message - ID: %s, MediaType: '%s'", + message.ID, message.MediaType)
♻️ Duplicate comments (1)
src/usecase/send.go (1)
873-999: Add bounded contexts to sticker conversions (still missing from the previous review).We’re still spawning
gif2webp/ffmpeg/convertwithexec.Command, so a hung codec will hang this goroutine indefinitely; the earlier review already called this out, but nothing changed. On top of that we still runffmpeg -versioneach time just to probe availability. Please pass a timeout context into these commands and rely onexec.LookPathfor existence checks.-func (service *serviceSend) convertGIFToAnimatedWebP(gifBytes []byte, absBaseDir string) ([]byte, error) { +func (service *serviceSend) convertGIFToAnimatedWebP(ctx context.Context, gifBytes []byte, absBaseDir string) ([]byte, error) { + convCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() … - if _, err := exec.LookPath("gif2webp"); err == nil { - cmd = exec.Command("gif2webp", "-q", "85", "-m", "4", gifTempPath, "-o", webpTempPath) + if _, err := exec.LookPath("gif2webp"); err == nil { + cmd = exec.CommandContext(convCtx, "gif2webp", "-q", "85", "-m", "4", gifTempPath, "-o", webpTempPath) if err := cmd.Run(); err == nil { converted = true } else { logrus.Warnf("gif2webp failed: %v", err) } } - if !converted && exec.Command("ffmpeg", "-version").Run() == nil { - cmd = exec.Command("ffmpeg", "-i", gifTempPath, "-vf", "scale=512:512:force_original_aspect_ratio=decrease", "-q:v", "5", "-y", webpTempPath) + if !converted { + if _, err := exec.LookPath("ffmpeg"); err == nil { + cmd = exec.CommandContext(convCtx, "ffmpeg", "-i", gifTempPath, "-vf", "scale=512:512:force_original_aspect_ratio=decrease", "-q:v", "5", "-y", webpTempPath) + if err := cmd.Run(); err == nil { + converted = true + } else { + logrus.Warnf("ffmpeg WebP conversion failed: %v", err) + } + } - if err := cmd.Run(); err == nil { - converted = true - } else { - logrus.Warnf("ffmpeg WebP conversion failed: %v", err) - } } if !converted { if _, err := exec.LookPath("convert"); err == nil { - cmd = exec.Command("convert", gifTempPath, "-quality", "85", webpTempPath) + cmd = exec.CommandContext(convCtx, "convert", gifTempPath, "-quality", "85", webpTempPath) if err := cmd.Run(); err == nil { converted = true } else { logrus.Warnf("ImageMagick conversion failed: %v", err) } } } … - stickerBytes, err = service.convertGIFToAnimatedWebP(inputFileBytes, absBaseDir) + stickerBytes, err = service.convertGIFToAnimatedWebP(ctx, inputFileBytes, absBaseDir)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
docker/golang.Dockerfile(1 hunks)src/domains/chatstorage/interfaces.go(0 hunks)src/infrastructure/chatstorage/sqlite_repository.go(2 hunks)src/infrastructure/whatsapp/init.go(1 hunks)src/usecase/send.go(25 hunks)
💤 Files with no reviewable changes (1)
- src/domains/chatstorage/interfaces.go
🚧 Files skipped from review as they are similar to previous changes (1)
- docker/golang.Dockerfile
| func (service *serviceSend) wrapAndStoreMessage(ctx context.Context, recipient types.JID, msg *waE2E.Message, content string, mediaInfo *domainChatStorage.MediaInfo) (whatsmeow.SendResponse, error) { | ||
| ts, err := whatsapp.GetClient().SendMessage(ctx, recipient, msg) | ||
| if err != nil { | ||
| return whatsmeow.SendResponse{}, err | ||
| } | ||
|
|
||
| // Store the sent message using chatstorage | ||
| senderJID := "" | ||
| if whatsapp.GetClient().Store.ID != nil { | ||
| senderJID = whatsapp.GetClient().Store.ID.String() | ||
| } | ||
|
|
||
| // Store message asynchronously with timeout | ||
| // Use a goroutine to avoid blocking the send operation | ||
| go func() { | ||
| storeCtx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | ||
| storeCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer cancel() | ||
|
|
||
| if err := service.chatStorageRepo.StoreSentMessageWithContext(storeCtx, ts.ID, senderJID, recipient.String(), content, ts.Timestamp); err != nil { | ||
| if errors.Is(err, context.DeadlineExceeded) { | ||
| logrus.Warn("Timeout storing sent message") | ||
| } else { | ||
| logrus.Warnf("Failed to store sent message: %v", err) | ||
| } | ||
| if err := service.storeMessage(storeCtx, ts.ID, senderJID, recipient.String(), content, ts.Timestamp, mediaInfo); err != nil { | ||
| logrus.Errorf("Failed to store sent message with media info: %v", err) | ||
| } | ||
| }() | ||
|
|
||
| return ts, nil | ||
| } | ||
|
|
||
| func (service serviceSend) SendText(ctx context.Context, request domainSend.MessageRequest) (response domainSend.GenericResponse, err error) { | ||
| func (service *serviceSend) storeMessage(ctx context.Context, messageID string, senderJID string, recipientJID string, content string, timestamp time.Time, mediaInfo *domainChatStorage.MediaInfo) error { | ||
| if service.chatStorageRepo == nil { | ||
| return fmt.Errorf("chat storage repository is not initialized") | ||
| } | ||
|
|
||
| jid, err := utils.ValidateJidWithLogin(whatsapp.GetClient(), recipientJID) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid recipient JID: %w", err) | ||
| } | ||
| chatJID := jid.String() | ||
|
|
||
| message := &domainChatStorage.Message{ | ||
| ID: messageID, | ||
| ChatJID: chatJID, | ||
| Sender: senderJID, | ||
| Content: content, | ||
| Timestamp: timestamp, | ||
| IsFromMe: true, | ||
| } | ||
|
|
||
| if mediaInfo != nil { | ||
| message.MediaType = mediaInfo.MediaType | ||
| message.Filename = mediaInfo.Filename | ||
| message.URL = mediaInfo.URL | ||
| message.MediaKey = mediaInfo.MediaKey | ||
| message.FileSHA256 = mediaInfo.FileSHA256 | ||
| message.FileEncSHA256 = mediaInfo.FileEncSHA256 | ||
| message.FileLength = mediaInfo.FileLength | ||
| } | ||
|
|
||
| return service.chatStorageRepo.StoreMessage(message) | ||
| } | ||
|
|
||
| func (service *serviceSend) SendText(ctx context.Context, request domainSend.MessageRequest) (response domainSend.GenericResponse, err error) { |
There was a problem hiding this comment.
Avoid revalidating the recipient JID inside storeMessage.
storeMessage still calls utils.ValidateJidWithLogin, so persisting a just-sent message depends on another network round-trip. If that lookup fails (temporary login/state issues, network blip, throttling), the send succeeds but the record is dropped—data loss. The caller already has the validated types.JID; reuse that instead of revalidating.
-func (service *serviceSend) wrapAndStoreMessage(ctx context.Context, recipient types.JID, msg *waE2E.Message, content string, mediaInfo *domainChatStorage.MediaInfo) (whatsmeow.SendResponse, error) {
+func (service *serviceSend) wrapAndStoreMessage(ctx context.Context, recipient types.JID, msg *waE2E.Message, content string, mediaInfo *domainChatStorage.MediaInfo) (whatsmeow.SendResponse, error) {
…
- if err := service.storeMessage(storeCtx, ts.ID, senderJID, recipient.String(), content, ts.Timestamp, mediaInfo); err != nil {
+ if err := service.storeMessage(storeCtx, ts.ID, recipient.String(), senderJID, content, ts.Timestamp, mediaInfo); err != nil {
logrus.Errorf("Failed to store sent message with media info: %v", err)
}
}()
…
-func (service *serviceSend) storeMessage(ctx context.Context, messageID string, senderJID string, recipientJID string, content string, timestamp time.Time, mediaInfo *domainChatStorage.MediaInfo) error {
+func (service *serviceSend) storeMessage(ctx context.Context, messageID string, chatJID string, senderJID string, content string, timestamp time.Time, mediaInfo *domainChatStorage.MediaInfo) error {
if service.chatStorageRepo == nil {
return fmt.Errorf("chat storage repository is not initialized")
}
-
- jid, err := utils.ValidateJidWithLogin(whatsapp.GetClient(), recipientJID)
- if err != nil {
- return fmt.Errorf("invalid recipient JID: %w", err)
- }
- chatJID := jid.String()
+
+ if chatJID == "" {
+ return fmt.Errorf("chat JID cannot be empty")
+ }🤖 Prompt for AI Agents
In src/usecase/send.go around lines 48 to 104, storeMessage re-validates the
recipient JID with utils.ValidateJidWithLogin causing an unnecessary network
call and potential data loss if that lookup fails; change storeMessage to accept
the already-validated chat JID (or types.JID) from the caller and remove the
ValidateJidWithLogin call and related error path, use the provided chatJID
directly when building the Message, and update the caller (wrapAndStoreMessage /
SendText) to pass the recipient’s validated jid.String() (or the types.JID) into
storeMessage so storing is local-only and does not depend on another network
lookup.
Signed-off-by: Baharsah <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/usecase/send.go (1)
128-133:getDefaultEphemeralExpirationis called with phone numbers instead of chat JIDs, so stored ephemeral settings are never used.
getDefaultEphemeralExpiration(jid string)looks up a chat by itsjidin the repository, but several call sites pass a raw phone (e.g.request.BaseRequest.Phone,request.Phone) instead of the canonical JID string that’s actually stored inchats.jid. That means these lookups almost always miss and you silently fall back to0even when the chat has a non-zeroEphemeralExpiration.Examples:
SendText:// uses phone, not JID msg.ExtendedTextMessage.ContextInfo.Expiration = proto.Uint32(service.getDefaultEphemeralExpiration(request.BaseRequest.Phone))
buildStickerMessage:ephemeralExpiration := service.getDefaultEphemeralExpiration(request.Phone)Suggested adjustments:
- In
SendText, use the already-validateddataWaRecipient:-} else { - msg.ExtendedTextMessage.ContextInfo.Expiration = proto.Uint32(service.getDefaultEphemeralExpiration(request.BaseRequest.Phone)) -} +} else { + msg.ExtendedTextMessage.ContextInfo.Expiration = + proto.Uint32(service.getDefaultEphemeralExpiration(dataWaRecipient.String())) +}
- For stickers, pass the chat JID into
buildStickerMessageinstead of recomputing fromrequest.Phone:- msg := service.buildStickerMessage(request, uploaded, processed) + msg := service.buildStickerMessage(request, recipient.String(), uploaded, processed)And update the helper:
-func (service *serviceSend) buildStickerMessage(request domainSend.StickerRequest, uploaded whatsmeow.UploadResponse, media *processedMedia) *waE2E.Message { +func (service *serviceSend) buildStickerMessage(request domainSend.StickerRequest, chatJID string, uploaded whatsmeow.UploadResponse, media *processedMedia) *waE2E.Message { @@ - ephemeralExpiration := service.getDefaultEphemeralExpiration(request.Phone) + ephemeralExpiration := service.getDefaultEphemeralExpiration(chatJID)With these changes, per-chat ephemeral configuration stored in
chats.ephemeral_expirationwill actually influence outgoing text and sticker messages.Also applies to: 171-175, 1239-1251, 1265-1280
♻️ Duplicate comments (2)
src/usecase/send.go (2)
48-69: Avoid re-validating recipient JID instoreMessageto prevent dropping already-sent messages.
wrapAndStoreMessagesends to a validatedrecipient types.JID, thenstoreMessagere-runsutils.ValidateJidWithLoginonrecipientJID. If that second validation fails (temporary login/network issue, rate limiting, account-validation disabled, etc.), the send has already succeeded but the DB write is skipped, causing silent data loss for sent messages. It also adds an unnecessary network round-trip.You already have a canonical chat JID in
recipient.String(). Pass that intostoreMessageand drop the extra validation:-func (service *serviceSend) wrapAndStoreMessage(ctx context.Context, recipient types.JID, msg *waE2E.Message, content string, mediaInfo *domainChatStorage.MediaInfo) (whatsmeow.SendResponse, error) { +func (service *serviceSend) wrapAndStoreMessage(ctx context.Context, recipient types.JID, msg *waE2E.Message, content string, mediaInfo *domainChatStorage.MediaInfo) (whatsmeow.SendResponse, error) { @@ - go func() { + go func() { storeCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - if err := service.storeMessage(storeCtx, ts.ID, senderJID, recipient.String(), content, ts.Timestamp, mediaInfo); err != nil { + if err := service.storeMessage(storeCtx, ts.ID, recipient.String(), senderJID, content, ts.Timestamp, mediaInfo); err != nil { logrus.Errorf("Failed to store sent message with media info: %v", err) } }() @@ -func (service *serviceSend) storeMessage(ctx context.Context, messageID string, senderJID string, recipientJID string, content string, timestamp time.Time, mediaInfo *domainChatStorage.MediaInfo) error { +func (service *serviceSend) storeMessage(ctx context.Context, messageID string, chatJID string, senderJID string, content string, timestamp time.Time, mediaInfo *domainChatStorage.MediaInfo) error { if service.chatStorageRepo == nil { return fmt.Errorf("chat storage repository is not initialized") } - - jid, err := utils.ValidateJidWithLogin(whatsapp.GetClient(), recipientJID) - if err != nil { - return fmt.Errorf("invalid recipient JID: %w", err) - } - chatJID := jid.String() + if chatJID == "" { + return fmt.Errorf("chat JID cannot be empty") + } @@ message := &domainChatStorage.Message{ ID: messageID, ChatJID: chatJID,This keeps storage purely local and removes the extra failure mode after a successful send.
Also applies to: 71-102
873-923: Add timeouts and cheaper availability checks for GIF→WebP conversion tools.
convertGIFToAnimatedWebPrunsgif2webp,ffmpeg, or ImageMagick viaexec.Commandwithout a context or timeout, and probes ffmpeg withexec.Command("ffmpeg", "-version"). A hung external process here can block the worker indefinitely, and the version probe is unnecessarily heavy. This mirrors a concern raised in an earlier review.You already use
context.WithTimeout+exec.CommandContextinprocessSticker’s static conversion branch; mirroring that here would make behavior consistent:-func (service *serviceSend) convertGIFToAnimatedWebP(gifBytes []byte, absBaseDir string) ([]byte, error) { +func (service *serviceSend) convertGIFToAnimatedWebP(ctx context.Context, gifBytes []byte, absBaseDir string) ([]byte, error) { @@ - var cmd *exec.Cmd - var converted bool + convCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + + var cmd *exec.Cmd + var converted bool @@ - if _, err := exec.LookPath("gif2webp"); err == nil { - cmd = exec.Command("gif2webp", "-q", "85", "-m", "4", gifTempPath, "-o", webpTempPath) + if _, err := exec.LookPath("gif2webp"); err == nil { + cmd = exec.CommandContext(convCtx, "gif2webp", "-q", "85", "-m", "4", gifTempPath, "-o", webpTempPath) @@ - if !converted && exec.Command("ffmpeg", "-version").Run() == nil { - cmd = exec.Command("ffmpeg", "-i", gifTempPath, "-vf", "scale=512:512:force_original_aspect_ratio=decrease", "-q:v", "5", "-y", webpTempPath) + if !converted { + if _, err := exec.LookPath("ffmpeg"); err == nil { + cmd = exec.CommandContext(convCtx, "ffmpeg", "-i", gifTempPath, "-vf", "scale=512:512:force_original_aspect_ratio=decrease", "-q:v", "5", "-y", webpTempPath) @@ - if !converted { - if _, err := exec.LookPath("convert"); err == nil { - cmd = exec.Command("convert", gifTempPath, "-quality", "85", webpTempPath) + if !converted { + if _, err := exec.LookPath("convert"); err == nil { + cmd = exec.CommandContext(convCtx, "convert", gifTempPath, "-quality", 85, webpTempPath)And update the call site in
processSticker:- stickerBytes, err = service.convertGIFToAnimatedWebP(inputFileBytes, absBaseDir) + stickerBytes, err = service.convertGIFToAnimatedWebP(ctx, inputFileBytes, absBaseDir)This prevents long-running or stuck conversions from tying up a goroutine indefinitely and makes the ffmpeg check cheaper.
Also applies to: 1095-1113
🧹 Nitpick comments (2)
src/infrastructure/chatstorage/sqlite_repository.go (1)
199-255: Consider toning down StoreMessage logging and avoid logging full content.
StoreMessagenow logs every call (including fullContent) at Info level plus additional Info/Warn/Error lines. In high-traffic deployments this can bloat logs and leak message bodies into log storage. Consider moving the detailed logs (especially withContent) to Debug level and/or truncating content, while keeping concise Info/Error logs around SQL execution and failures.src/usecase/send.go (1)
190-378: Only stickers populate media metadata; other media sends are stored as text-only.All media senders now route through
wrapAndStoreMessage, butSendImage,SendFile,SendVideo,SendContact,SendLink,SendLocation,SendAudio, andSendPollalways passmediaInfo == nil. As a result, stored rows for outgoing media havemedia_type, URL, key, hashes, etc. empty and look like plain text messages (with emoji prefixes).If the intent is symmetric media history (so filters like
HasMediaor future re-download features work for both incoming and outgoing messages), consider populatingMediaInfoin these paths similarly toSendSticker:
- For images/videos/documents/audio/links, build a
MediaInfofrom the correspondinguploadedresponse (URL, MediaKey, SHA256, EncSHA256, FileLength, and an appropriateMediaTypestring), and pass it intowrapAndStoreMessage.If this asymmetry is intentional (e.g., you only care about metadata for stickers right now), you can leave as-is, but it’s good to be aware of the difference in stored data.
Also applies to: 395-548, 550-591, 593-662, 664-705, 707-807
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
docker/golang.Dockerfile(1 hunks)src/infrastructure/chatstorage/sqlite_repository.go(2 hunks)src/infrastructure/whatsapp/init.go(1 hunks)src/usecase/send.go(25 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker/golang.Dockerfile
🧰 Additional context used
🧬 Code graph analysis (2)
src/infrastructure/whatsapp/init.go (2)
src/domains/chatstorage/chatstorage.go (1)
Message(16-32)src/config/settings.go (1)
WhatsappAutoReplyMessage(28-28)
src/usecase/send.go (6)
src/domains/chatstorage/chatstorage.go (2)
Message(16-32)MediaInfo(35-45)src/pkg/utils/general.go (2)
ContainsMention(220-233)DownloadImageFromURL(235-285)src/domains/send/sticker.go (1)
StickerRequest(5-9)src/pkg/error/whatsapp_error.go (1)
WaUploadMediaError(56-56)src/config/settings.go (1)
PathSendItems(21-21)src/pkg/error/generic_error.go (1)
InternalServerError(12-12)
🔇 Additional comments (3)
src/infrastructure/whatsapp/init.go (1)
772-787: Auto-reply storage via StoreMessage is wired correctly.The new path that builds a
domainChatStorage.Messagefor auto-replies and persists it withStoreMessageuses the expected fields (ID, ChatJID, Sender, Content, Timestamp, IsFromMe) and avoids re-validating JIDs. This aligns well with the updated storage model.src/usecase/send.go (2)
863-871: HelpersgetMentionFromText,uploadMedia, andgetDefaultEphemeralExpirationare structured cleanly.
getMentionFromTextcleanly reusesutils.ContainsMentionand normalizes each phone to a JID viaValidateJidWithLogin, which keeps mentions consistent with other message flows.uploadMediacentralizes the distinction between regular and newsletter uploads behind a small helper, which simplifies call sites.- Aside from the phone-vs-JID issue already mentioned,
getDefaultEphemeralExpiration’s behavior (fail-closed to0on errors) is a reasonable, safe default.No changes needed here beyond the earlier suggestion about passing proper chat JIDs into
getDefaultEphemeralExpiration.Also applies to: 1256-1281
960-1004: Sticker send pipeline and metadata storage look solid overall.The new
SendSticker/processSticker/buildStickerMessageflow does a good job of:
- Supporting both uploaded files and URLs with temp-file cleanup.
- Normalizing stickers to WebP (animated or static) and enforcing reasonable dimension bounds.
- Persisting sticker media metadata (type
"sticker", URL, MediaKey, hashes, length) viaMediaInfo, so DB rows for stickers are rich enough for later querying.Once the earlier points about GIF conversion timeouts and ephemeral JID lookups are addressed, this will be a robust implementation.
Also applies to: 1224-1253
| func (service *serviceSend) getStickerPath(request domainSend.StickerRequest) (string, func(), error) { | ||
| absBaseDir, err := filepath.Abs(config.PathSendItems) | ||
| if err != nil { | ||
| return response, pkgError.InternalServerError(fmt.Sprintf("failed to resolve base directory: %v", err)) | ||
| return "", nil, pkgError.InternalServerError(fmt.Sprintf("failed to resolve base directory: %v", err)) | ||
| } | ||
|
|
||
| defer func() { | ||
| // Delete temporary files | ||
| var stickerPath string | ||
| var deletedItems []string | ||
| cleanup := func() { | ||
| for _, path := range deletedItems { | ||
| if err := os.Remove(path); err != nil && !os.IsNotExist(err) { | ||
| logrus.Warnf("Failed to cleanup temporary file %s: %v", path, err) | ||
| } | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| // Handle sticker from URL or file | ||
| if request.StickerURL != nil && *request.StickerURL != "" { | ||
| // Download sticker from URL | ||
| imageData, _, err := utils.DownloadImageFromURL(*request.StickerURL) | ||
| if err != nil { | ||
| return response, pkgError.InternalServerError(fmt.Sprintf("failed to download sticker from URL: %v", err)) | ||
| return "", nil, pkgError.InternalServerError(fmt.Sprintf("failed to download sticker from URL: %v", err)) | ||
| } | ||
|
|
||
| // Create safe temporary file within base dir | ||
| f, err := os.CreateTemp(absBaseDir, "sticker_*") | ||
| if err != nil { | ||
| return response, pkgError.InternalServerError(fmt.Sprintf("failed to create temp file: %v", err)) | ||
| return "", nil, pkgError.InternalServerError(fmt.Sprintf("failed to create temp file: %v", err)) | ||
| } | ||
| stickerPath = f.Name() | ||
| if _, err := f.Write(imageData); err != nil { | ||
| f.Close() | ||
| return response, pkgError.InternalServerError(fmt.Sprintf("failed to write sticker: %v", err)) | ||
| cleanup() | ||
| return "", nil, pkgError.InternalServerError(fmt.Sprintf("failed to write sticker: %v", err)) | ||
| } | ||
| _ = f.Close() | ||
| deletedItems = append(deletedItems, stickerPath) | ||
| } else if request.Sticker != nil { | ||
| // Create safe temporary file within base dir | ||
| f, err := os.CreateTemp(absBaseDir, "sticker_*") | ||
| if err != nil { | ||
| return response, pkgError.InternalServerError(fmt.Sprintf("failed to create temp file: %v", err)) | ||
| return "", nil, pkgError.InternalServerError(fmt.Sprintf("failed to create temp file: %v", err)) | ||
| } | ||
| stickerPath = f.Name() | ||
| _ = f.Close() | ||
|
|
||
| // Save uploaded file to safe path | ||
| err = fasthttp.SaveMultipartFile(request.Sticker, stickerPath) | ||
| if err != nil { | ||
| return response, pkgError.InternalServerError(fmt.Sprintf("failed to save sticker: %v", err)) | ||
| cleanup() | ||
| return "", nil, pkgError.InternalServerError(fmt.Sprintf("failed to save sticker: %v", err)) | ||
| } | ||
| deletedItems = append(deletedItems, stickerPath) | ||
| } | ||
|
|
||
| // Convert image to WebP format for sticker (512x512 max size) | ||
| srcImage, err := imaging.Open(stickerPath) | ||
| return stickerPath, cleanup, nil | ||
| } |
There was a problem hiding this comment.
Ensure temporary sticker files are cleaned up on write/save errors.
getStickerPath tracks files to delete in deletedItems, but in both branches you only append to deletedItems after successfully writing/saving. On errors, you call cleanup() while deletedItems is still empty, so the temp file created by os.CreateTemp is leaked.
To guarantee cleanup on failures, register the path before the write/save:
// URL-based sticker
- stickerPath = f.Name()
- if _, err := f.Write(imageData); err != nil {
- f.Close()
- cleanup()
- return "", nil, pkgError.InternalServerError(fmt.Sprintf("failed to write sticker: %v", err))
- }
- _ = f.Close()
- deletedItems = append(deletedItems, stickerPath)
+ stickerPath = f.Name()
+ deletedItems = append(deletedItems, stickerPath)
+ if _, err := f.Write(imageData); err != nil {
+ f.Close()
+ cleanup()
+ return "", nil, pkgError.InternalServerError(fmt.Sprintf("failed to write sticker: %v", err))
+ }
+ _ = f.Close()
@@
} else if request.Sticker != nil {
f, err := os.CreateTemp(absBaseDir, "sticker_*")
if err != nil {
return "", nil, pkgError.InternalServerError(fmt.Sprintf("failed to create temp file: %v", err))
}
- stickerPath = f.Name()
- _ = f.Close()
-
- err = fasthttp.SaveMultipartFile(request.Sticker, stickerPath)
- if err != nil {
- cleanup()
- return "", nil, pkgError.InternalServerError(fmt.Sprintf("failed to save sticker: %v", err))
- }
- deletedItems = append(deletedItems, stickerPath)
+ stickerPath = f.Name()
+ deletedItems = append(deletedItems, stickerPath)
+ _ = f.Close()
+
+ err = fasthttp.SaveMultipartFile(request.Sticker, stickerPath)
+ if err != nil {
+ cleanup()
+ return "", nil, pkgError.InternalServerError(fmt.Sprintf("failed to save sticker: %v", err))
+ }This keeps the filesystem clean even when Download/Save fails.
🤖 Prompt for AI Agents
In src/usecase/send.go around lines 1006 to 1057, temp files created with
os.CreateTemp are only appended to deletedItems after a successful write/save,
so calling cleanup on write/save errors won't remove the leaked temp file;
change the logic to append the temp file path to deletedItems immediately after
creating the temp file (before any Write/Save), ensure the file is closed as
needed, and keep calling cleanup on error paths so the temporary file is removed
even when Write/Save fails.
1 participant
No description provided.