Fix: attachment issues #913
Conversation
|
""" WalkthroughThis pull request introduces several enhancements across the codebase. In the API layer, the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ChannelHandler
participant Logger
Client->>ChannelHandler: getPublicUrl(attachment)
alt Attachment missing or invalid
ChannelHandler-->>Client: Return fallback URL
else Attachment provided
ChannelHandler->>ChannelHandler: Lookup resource
alt Resource found
ChannelHandler-->>Client: Return constructed download URL
else Resource missing
ChannelHandler->>Logger: Log warning
ChannelHandler-->>Client: Return fallback URL
end
end
sequenceDiagram
participant Event
participant MessageService
participant AttachmentService
participant MessageRepository
participant Logger
Event->>MessageService: handleDeleteImage(query, criteria)
MessageService->>AttachmentService: find(criteria)
alt Attachment exists
MessageService->>MessageRepository: updateMultiple (set payload.id to null)
MessageRepository-->>MessageService: Update result
else Attachment not found or error
MessageService->>Logger: Log error
end
Assessment against linked issues
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
api/src/setting/services/setting.service.ts (1)
50-68: Consider finalizing or removing commented code for avatar setup.The commented-out code appears to be an implementation for setting up a default avatar image. If this functionality is needed, it should be properly implemented and the code uncommented. Otherwise, consider removing it to maintain code cleanliness.
Note that if you do uncomment and implement this code, you'll need to:
- Import necessary dependencies (fs, path)
- Inject the attachmentService in the constructor
- Import the required types (AttachmentAccess, AttachmentCreatedByRef, AttachmentResourceRef)
widget/src/components/messages/FileMessage.scss (2)
13-21: New CSS Error Message Styling IntroducedThe new
.error-messageclass adds a clear style for error feedback within file messages with appropriate margin, padding, border-radius, color, width, and display properties. Ensure that its design aligns with the overall UI/UX guidelines and that the consumption of this class in the React component (FileMessage.tsx) is updated accordingly.
77-85: Legacy Commented-Out Styling BlockA previously used or alternative version of the
.error-messageclass is still present but commented out. If this code is no longer needed, please consider removing it to keep the stylesheet clean; otherwise, add a clarifying comment as to why it remains in the file.api/src/channel/lib/Handler.ts (1)
333-336: Remove commented-out code.The code now correctly returns a fallback URL, but there's a commented-out exception throw that should be removed as it's no longer needed and could cause confusion during future maintenance.
return buildURL(config.apiBaseUrl, `/webhook/${name}/not-found`); - // throw new TypeError('Unable to resolve the attachment public URL.');api/src/chat/services/message.service.ts (1)
159-166: Consider the performance implications of the updateMany operation.The current implementation uses
updateManyto update all messages that reference the deleted attachment. Depending on the volume of messages, this could be an expensive operation.If message volume is high, consider adding an index on
message.attachment.payload.idto improve query performance, or implementing a batched update approach for very large message collections.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/assets/hexavatar.pngis excluded by!**/*.png
📒 Files selected for processing (14)
api/src/attachment/controllers/attachment.controller.ts(1 hunks)api/src/attachment/types/index.ts(1 hunks)api/src/channel/lib/Handler.ts(2 hunks)api/src/channel/webhook.controller.ts(1 hunks)api/src/chat/chat.module.ts(4 hunks)api/src/chat/services/message.service.ts(3 hunks)api/src/setting/services/setting.service.ts(1 hunks)frontend/public/locales/en/translation.json(1 hunks)frontend/public/locales/fr/translation.json(1 hunks)frontend/src/components/inbox/components/AttachmentViewer.tsx(4 hunks)widget/src/components/messages/FileMessage.scss(2 hunks)widget/src/components/messages/FileMessage.tsx(4 hunks)widget/src/translations/en/translation.json(1 hunks)widget/src/translations/fr/translation.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
api/src/channel/webhook.controller.ts (1)
frontend/src/components/roles/index.tsx (1)
Roles(38-162)
api/src/chat/chat.module.ts (1)
api/src/attachment/schemas/attachment.schema.ts (1)
AttachmentModel(168-171)
frontend/src/components/inbox/components/AttachmentViewer.tsx (1)
api/src/i18n/services/i18n.service.ts (1)
t(27-57)
🪛 Biome (1.9.4)
api/src/attachment/controllers/attachment.controller.ts
[error] 136-136: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Frontend Tests
- GitHub Check: API-Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (30)
api/src/channel/webhook.controller.ts (1)
91-96: Good addition of a 'not-found' error handling endpoint.This new endpoint appropriately returns a 404 status with a clear error message, which will help with graceful handling of cases where a channel is not found. The public role ensures it's accessible to external services.
api/src/attachment/types/index.ts (1)
18-18: Good addition of 'System' to AttachmentCreatedByRef enum.This enhancement allows for system-generated attachments, which is a valuable addition for distinguishing between user, subscriber, and system-created content.
frontend/public/locales/en/translation.json (1)
119-123: New Error Message Keys AddedFour new keys—
"image_error","file_error","audio_error", and"video_error"—have been introduced to provide clear error messages for missing media attachments. Verify that these keys are consistently referenced in the UI components and that the trailing commas maintain valid JSON syntax.widget/src/translations/en/translation.json (1)
20-24: Consistent Enhancement of Error MessagingThe addition of error message keys within the
"file_message"object (i.e.,"image_error","file_error","audio_error", and"video_error") aligns well with the attachment issue being fixed. Confirm that the UI components refer to these keys for displaying error messages.frontend/public/locales/fr/translation.json (1)
120-123: New French Error Message Keys AddedThe new keys for attachment issues—
"image_error","file_error","audio_error", and"video_error"—are now provided in French. Ensure that these keys are used throughout the French UI and that they match the keys in the English version for consistency.widget/src/translations/fr/translation.json (1)
119-124: Enhanced Error Messaging in FrenchThis update adds localized error messages for attachments under the
"file_message"block. The new keys ("image_error","file_error","audio_error","video_error") are clearly translated and should improve the user experience when media-related issues arise.api/src/chat/chat.module.ts (4)
2-2: LGTM: Updated copyright year.The copyright year has been updated to 2025.
14-16: LGTM: Added necessary imports for attachment functionality.These imports are required for the attachment integration in the chat module.
64-64: LGTM: Added AttachmentModel to MongooseModule.forFeature.This change allows MongoDB to recognize and manage the Attachment schema.
100-101: LGTM: Added attachment services to providers.AttachmentService and AttachmentRepository are now properly registered as providers, making them available for dependency injection within the module.
frontend/src/components/inbox/components/AttachmentViewer.tsx (6)
11-11: LGTM: Updated import to include useState.The useState hook is now properly imported for managing error states.
33-38: LGTM: Added error handling for image attachments.This change improves user experience by displaying an appropriate error message when an image fails to load or when no URL is provided.
43-43: LGTM: Added error event handling for images.The onError event handler now sets the imageErrored state when an image fails to load.
68-77: LGTM: Implemented error handling for audio attachments.This change introduces robust error handling for audio playback failures, showing a user-friendly error message when appropriate.
82-84: LGTM: Added URL validation for file attachments.This validation ensures a meaningful error message is displayed when a file URL is missing.
106-119: LGTM: Added error handling for video attachments.Similar to other media types, video playback now has proper error handling to improve user experience when videos cannot be loaded.
api/src/channel/lib/Handler.ts (3)
311-311: LGTM: Simplified variable declaration.The code now uses destructuring to extract just the name from the split result, ignoring the suffix.
313-315: LGTM: Added null check for attachment.This change prevents potential errors by checking if the attachment or its ID exists before proceeding, returning a fallback URL if not.
320-322: LGTM: Improved error handling for missing attachments.Rather than throwing an exception, the code now logs a warning and returns a fallback URL, making the application more resilient.
widget/src/components/messages/FileMessage.tsx (9)
2-2: LGTM: Updated copyright year.The copyright year has been updated to 2025.
9-9: LGTM: Updated import to include useState.The useState hook is properly imported for managing error states.
25-27: LGTM: Added state variables for error handling.These state variables enable tracking of loading errors for different media types.
32-34: LGTM: Added handling for unknown file types.This provides a user-friendly message when a file type is unknown, improving the user experience.
42-43: LGTM: Fixed typo in error message.The error message spelling has been corrected from "Uknown" to "Unknown".
55-71: LGTM: Implemented error handling for image display.This change adds robust error handling for images, showing a styled error message when an image fails to load.
76-89: LGTM: Added error handling for audio playback.Similar to the image implementation, this adds proper error handling for audio elements.
94-108: LGTM: Added error handling for video playback.The code now properly handles video loading errors, displaying a styled error message when appropriate.
119-133: LGTM: Added URL validation for file downloads.This validation ensures users receive a clear error message when a file URL is missing instead of getting a broken download link.
api/src/chat/services/message.service.ts (2)
14-21: Import dependencies are correctly structured for the new attachment handling feature.The new imports are appropriately added to support the attachment deletion functionality. The
OnEventdecorator from NestJS event emitter will allow the service to respond to attachment deletion events.
48-48: Constructor properly injects the AttachmentService dependency.The
AttachmentServiceis correctly injected as a private field, maintaining the existing pattern of dependency injection in the class.
| @OnEvent('hook:attachment:preDelete') | ||
| async handleDeleteImage( | ||
| _query: Query< | ||
| DeleteResult, | ||
| Document<Attachment, any, any>, | ||
| unknown, | ||
| Attachment, | ||
| 'deleteOne' | 'deleteMany' | ||
| >, | ||
| criteria: TFilterQuery<Attachment>, | ||
| ) { | ||
| // todo: handle deleteMany | ||
| try { | ||
| this.logger.log( | ||
| 'deleting attachment messages containing deleted images', | ||
| criteria, | ||
| ); | ||
| const foundAttachment = await this.attachmentService.findOne(criteria); | ||
| if (!foundAttachment) { | ||
| return; | ||
| } | ||
| await this.updateMany( | ||
| { | ||
| 'message.attachment.payload.id': criteria._id, | ||
| }, | ||
| { | ||
| ['message.attachment.payload.id' as any]: null, | ||
| }, | ||
| ); | ||
| } catch (error) { | ||
| this.logger.error( | ||
| 'Unable to cleanup old messages with attachment ids', | ||
| error, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The new event handler addresses attachment deletion, but has several improvement opportunities.
The handleDeleteImage method correctly handles cleaning up messages when attachments are deleted, but there are a few issues to address:
- The TODO comment indicates incomplete functionality for handling multiple deletions
- The
_queryparameter is unused but still present - There's a type casting to
anythat could be improved - Error handling just logs errors without any recovery mechanism
Consider these improvements:
@OnEvent('hook:attachment:preDelete')
async handleDeleteImage(
- _query: Query<
+ query: Query<
DeleteResult,
Document<Attachment, any, any>,
unknown,
Attachment,
'deleteOne' | 'deleteMany'
>,
criteria: TFilterQuery<Attachment>,
) {
- // todo: handle deleteMany
+ // Handle both single and batch deletions
try {
this.logger.log(
'deleting attachment messages containing deleted images',
criteria,
);
const foundAttachment = await this.attachmentService.findOne(criteria);
if (!foundAttachment) {
return;
}
await this.updateMany(
{
'message.attachment.payload.id': criteria._id,
},
{
- ['message.attachment.payload.id' as any]: null,
+ $set: { 'message.attachment.payload.id': null },
},
);
} catch (error) {
this.logger.error(
'Unable to cleanup old messages with attachment ids',
error,
);
+ // Consider adding metrics or alerts for monitoring these failures
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @OnEvent('hook:attachment:preDelete') | |
| async handleDeleteImage( | |
| _query: Query< | |
| DeleteResult, | |
| Document<Attachment, any, any>, | |
| unknown, | |
| Attachment, | |
| 'deleteOne' | 'deleteMany' | |
| >, | |
| criteria: TFilterQuery<Attachment>, | |
| ) { | |
| // todo: handle deleteMany | |
| try { | |
| this.logger.log( | |
| 'deleting attachment messages containing deleted images', | |
| criteria, | |
| ); | |
| const foundAttachment = await this.attachmentService.findOne(criteria); | |
| if (!foundAttachment) { | |
| return; | |
| } | |
| await this.updateMany( | |
| { | |
| 'message.attachment.payload.id': criteria._id, | |
| }, | |
| { | |
| ['message.attachment.payload.id' as any]: null, | |
| }, | |
| ); | |
| } catch (error) { | |
| this.logger.error( | |
| 'Unable to cleanup old messages with attachment ids', | |
| error, | |
| ); | |
| } | |
| } | |
| @OnEvent('hook:attachment:preDelete') | |
| async handleDeleteImage( | |
| query: Query< | |
| DeleteResult, | |
| Document<Attachment, any, any>, | |
| unknown, | |
| Attachment, | |
| 'deleteOne' | 'deleteMany' | |
| >, | |
| criteria: TFilterQuery<Attachment>, | |
| ) { | |
| // Handle both single and batch deletions | |
| try { | |
| this.logger.log( | |
| 'deleting attachment messages containing deleted images', | |
| criteria, | |
| ); | |
| const foundAttachment = await this.attachmentService.findOne(criteria); | |
| if (!foundAttachment) { | |
| return; | |
| } | |
| await this.updateMany( | |
| { | |
| 'message.attachment.payload.id': criteria._id, | |
| }, | |
| { | |
| $set: { 'message.attachment.payload.id': null }, | |
| }, | |
| ); | |
| } catch (error) { | |
| this.logger.error( | |
| 'Unable to cleanup old messages with attachment ids', | |
| error, | |
| ); | |
| // Consider adding metrics or alerts for monitoring these failures | |
| } | |
| } |
abdou6666
requested review from
medchedli,
yassinedorbozgithub and
MohamedAliBouhaouala
There was a problem hiding this comment.
Hi @abdou6666
I noticed this behavior when triggering an attachment block missing an attachment.
Screencast from 04-16-2025 02:19:13 PM.webm
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
widget/src/components/messages/FileMessage.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
widget/src/components/messages/FileMessage.tsx
[error] 41-42: There is a suspicious semicolon in the JSX element.
This is usually the result of a typo or some refactor gone wrong.
Remove the semicolon, or move it inside a JSX element.
(lint/suspicious/noSuspiciousSemicolonInJsx)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: API-Tests
- GitHub Check: Frontend Tests
🔇 Additional comments (7)
widget/src/components/messages/FileMessage.tsx (7)
9-9: Import of useState correctly addedAdding useState is necessary to support the new error state variables for tracking media loading failures.
25-27: Good addition of error state trackingThese state variables provide a clean way to track loading errors for different media types, which is essential for the robust error handling being implemented.
52-52: Typo correction in error messageFixed typo from "Uknown" to "Unknown" in the error message, improving code clarity.
65-81: Robust error handling for imagesThis implementation correctly handles image loading failures by:
- Setting an error state when the image fails to load
- Displaying a user-friendly error message with appropriate styling
- Maintaining consistent styling with the parent component
This addresses the PR objective of preventing crashes when attachments are deleted.
86-99: Robust error handling for audioThe audio error handling implementation follows the same pattern as images - capturing the error event and showing a localized error message.
104-118: Robust error handling for videosThe video error handling follows the consistent pattern established for other media types, with proper error state management and user feedback.
129-143: Proper URL validation for file downloadsThis change correctly handles missing URLs for file downloads, showing an error message instead of trying to create a download link to an invalid resource, which aligns perfectly with the PR objective of fixing attachment issues.
|
|
| if (message.data.type === "unknown") { | ||
| return ( | ||
| <div | ||
| className="sc-message--file" | ||
| style={{ | ||
| color: colors.text, | ||
| backgroundColor: colors.bg, | ||
| }} | ||
| > | ||
| <p className="error-message"> | ||
| {t("messages.file_message.unsupported_file_type")} | ||
| </p> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Hi @abdou6666, I suggest to create a function to handle the message.data.type values.
The intent is to centralize the check in one place.
Please, don't hesitate to ask if you have any questions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
widget/src/components/messages/FileMessage.tsx (2)
108-124: Consider adding a browser fallback message for audio.While you've added good error handling for audio elements, consider adding a browser compatibility message similar to what you've done for video (line 140).
<audio controls onError={() => setAudioErrored(true)}> <source src={hasUrl(message) ? message.data.url : ""} /> + {t("messages.file_message.browser_audio_unsupport")} </audio>
17-168: Consider refactoring repeated error styling patterns.There's some duplication in how error messages are styled across different media types. Consider extracting a reusable error message component to reduce duplication.
// Example of a reusable error component const MediaErrorMessage = ({ message, backgroundColor }: { message: string, backgroundColor: string }) => ( <p className="error-message" style={{ backgroundColor, }} > {message} </p> ); // Then usage would be: {imageErrored ? ( <MediaErrorMessage message={t("messages.file_message.image_error")} backgroundColor={colors.bg} /> ) : ( <img ... /> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
widget/src/components/messages/FileMessage.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Frontend Tests
- GitHub Check: API-Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
widget/src/components/messages/FileMessage.tsx (9)
2-2: Verify the copyright year update.The copyright year has been updated to 2025. Please verify if this is intentional, as we're currently in 2024.
9-9: Good addition of useState import.Adding the useState import is necessary for the new error state management you've implemented.
17-35: Well-structured type safety function implementation.The
getFileMessageTypefunction provides robust type checking with clear error handling, addressing the previous reviewer's suggestion to centralize file type checking. It properly validates allowed types and handles unknown types gracefully.
36-48: Excellent type guard implementation.The
hasUrlfunction uses type guards effectively to ensure safe access to the URL property, preventing runtime errors when attachments are deleted or invalid. This directly addresses the core issue in this PR.
58-61: Good implementation of error state management.Adding dedicated state variables for each media type's error state allows for granular error handling, improving the user experience when media fails to load.
63-77: Effective early return pattern for unknown file types.Using an early return pattern for handling unknown file types is a good practice that improves code readability and provides a clear error message to users.
87-106: Robust image error handling implementation.The conditional rendering based on the
imageErroredstate effectively prevents the component from crashing when image attachments are missing or invalid.
126-143: Good video error handling with browser compatibility message.The video element now properly handles errors and includes a fallback message for browsers that don't support the video format.
145-168: Effective file download error handling.The conditional rendering for file downloads prevents errors when the file URL is missing, providing a clear error message instead of attempting to create a broken download link.
|
While testing a different PR I realized that we need to handle List Block / Custom Plugins might have stale attachment Id in the messages) |
Motivation
This PR addresses the issue of console widget crushing when an attachment in attachment block gets deleted & subscriber has that attachment in their conversation history
Fixes #895
Type of change:
Please delete options that are not relevant.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Style