fix: description being used as alternative text for images#40839
Merged
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 55a9756 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
WalkthroughRename image attachment metadata from ChangesAlt-text naming migration
Sequence DiagramsequenceDiagram
participant UploadsStore
participant ProcessUploads
participant SendFileMessage
participant MessageRenderer
participant ImageAttachment
UploadsStore->>ProcessUploads: upload with altText
ProcessUploads->>SendFileMessage: attachment with image_alt
SendFileMessage->>MessageRenderer: message with image_alt
MessageRenderer->>ImageAttachment: attachment object
ImageAttachment->>User: renders img with alt from image_alt
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40839 +/- ##
===========================================
- Coverage 69.85% 69.85% -0.01%
===========================================
Files 3349 3349
Lines 124373 124374 +1
Branches 22163 22219 +56
===========================================
- Hits 86878 86877 -1
+ Misses 34145 34140 -5
- Partials 3350 3357 +7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
7 issues found across 51 files
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
apps/meteor/client/lib/parseMessageTextToAstMarkdown.spec.ts (1)
403-488: ⚡ Quick winAdd a regression test for file attachments containing both
textanddescription.Current additions validate translated
md, but they don’t pin thedescriptionMdsource when both fields are present. A focused case here would prevent the file-caption markdown source from regressing again.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/client/lib/parseMessageTextToAstMarkdown.spec.ts` around lines 403 - 488, Add a regression test that creates a file-type attachment object containing both text and description (optionally with translation fields) and asserts parseMessageAttachments(...) returns the description-based markdown in the descriptionMd field (not the text). Specifically, add a test case near the other attachment tests that calls parseMessageAttachments with an attachment where both text and description are set and then expect the returned [0].descriptionMd to strictly equal the parsed AST for the description value (use same Root shape as other tests); reference parseMessageAttachments and the descriptionMd property to locate where to assert.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/app/autotranslate/client/lib/autotranslate.ts`:
- Around line 72-80: The new description-translation branch can run after the
attachment.text branch and reuse attachment.translations[language] and
attachment.translations.original, causing both fields to be translated and the
original to be clobbered; update the logic so description and text are mutually
exclusive: if attachment.description exists prefer handling it (and skip text),
or if you choose to run text first then short-circuit and do not execute the
description branch when attachment.text was already processed; adjust checks
around attachment.text, attachment.description, attachment.translations,
language and autoTranslateShowInverse so only one of those branches mutates
attachment.translations.original or attachment.* per pass.
In `@apps/meteor/app/lib/server/methods/updateMessage.ts`:
- Around line 90-94: The update currently unconditionally sets
originalMessage.attachments[0].description = message.msg which can erase
existing captions when the update payload doesn't include text; change the logic
in updateMessage (use the originalMessage and message variables) so you only
overwrite attachments[0].description when message.msg is present (e.g., !==
undefined and not an empty string) in the incoming update; keep the assignment
of message.attachments and the swap of message.msg/message.msg restoration
scoped to that same guard so non-text edits don't clear existing descriptions.
In `@apps/meteor/app/livechat/server/lib/sendTranscript.ts`:
- Line 120: The call to escapeHtml currently discards its return value, leaving
messageContent unescaped and opening an XSS vector; update the sendTranscript
flow so the escaped string is used when building the HTML (e.g., assign the
result of escapeHtml(messageContent) back to messageContent or to a new variable
and use that variable in the template). Locate the escapeHtml invocation and the
HTML interpolation in the sendTranscript function (in sendTranscript.ts) and
replace the discarded call with an assignment so the sanitized value is what
gets inserted into the transcript HTML.
- Around line 119-120: The code currently overwrites messageContent with
message.attachments[0].description for any message; change it to only use the
attachment description when the message is not a system message so
already-formatted/sanitized system messages (the content produced earlier with
DOMPurify) are preserved. Concretely, check the message type/flag that
identifies system messages (e.g., a property like message.system or message.type
=== 'system') before assigning messageContent =
message.attachments[0].description, and keep the existing DOMPurify-sanitized
messageContent for system messages; still call escapeHtml(messageContent) after
deciding which content to use. Ensure you update the logic around
messageContent, message.attachments, and the escapeHtml call so system messages
are never overwritten by attachment descriptions.
In `@apps/meteor/app/slackbridge/server/RocketAdapter.ts`:
- Around line 206-216: The code can produce "undefined <fileName>" when
rocketMessage.msg and attachment.description are both missing; in RocketAdapter
(around getMessageAttachment and the slack.postMessage call) initialize text to
an empty string (e.g., text = rocketMessage.msg || '') and when setting from the
attachment only use attachment.description if present (e.g., if (!text &&
attachment.description) text = attachment.description), then build the msg
payload passed to slack.postMessage (slack.postMessage(..., { ...rocketMessage,
msg: `${text} ${fileName}` })) so it never interpolates undefined.
In `@apps/meteor/client/hooks/useDecryptedMessage.spec.ts`:
- Line 66: The final assertion in the useDecryptedMessage test is synchronous
and races the async decrypt flow; change the immediate
expect(result.current).toBe('Attachment description') to be wrapped in a waitFor
so the assertion waits for state updates from the hook (use the
testing-library's waitFor and assert inside it, e.g. waitFor(() =>
expect(result.current).toBe(...))). Update the test in
useDecryptedMessage.spec.ts where result.current is asserted to use waitFor to
make the check reliable.
In `@apps/meteor/client/lib/parseMessageTextToAstMarkdown.ts`:
- Around line 75-79: The current logic sets attachment.descriptionMd by calling
textToMessageToken(text, parseOptions) which may use attachment.text instead of
the actual attachment.description; update the branch inside the isFileAttachment
block (the conditional that sets attachment.descriptionMd) to call
textToMessageToken(attachment.description, parseOptions) (while preserving the
translated || isEncryptedMessageAttachment(attachment) condition and the
existing fallback to attachment.descriptionMd) so the markdown is parsed from
attachment.description itself.
---
Nitpick comments:
In `@apps/meteor/client/lib/parseMessageTextToAstMarkdown.spec.ts`:
- Around line 403-488: Add a regression test that creates a file-type attachment
object containing both text and description (optionally with translation fields)
and asserts parseMessageAttachments(...) returns the description-based markdown
in the descriptionMd field (not the text). Specifically, add a test case near
the other attachment tests that calls parseMessageAttachments with an attachment
where both text and description are set and then expect the returned
[0].descriptionMd to strictly equal the parsed AST for the description value
(use same Root shape as other tests); reference parseMessageAttachments and the
descriptionMd property to locate where to assert.
🪄 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: 4083b201-8379-47d4-9651-8af8a44e63f9
📒 Files selected for processing (51)
apps/meteor/app/autotranslate/client/lib/autotranslate.tsapps/meteor/app/autotranslate/server/autotranslate.tsapps/meteor/app/autotranslate/server/deeplTranslate.tsapps/meteor/app/autotranslate/server/googleTranslate.tsapps/meteor/app/autotranslate/server/msTranslate.tsapps/meteor/app/file-upload/server/methods/sendFileMessage.tsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/app/lib/server/methods/updateMessage.tsapps/meteor/app/livechat/server/lib/sendTranscript.tsapps/meteor/app/slackbridge/server/RocketAdapter.tsapps/meteor/app/ui/client/lib/ChatMessages.tsapps/meteor/client/components/message/content/attachments/DefaultAttachment.tsxapps/meteor/client/components/message/content/attachments/file/AudioAttachment.tsxapps/meteor/client/components/message/content/attachments/file/GenericFileAttachment.tsxapps/meteor/client/components/message/content/attachments/file/ImageAttachment.tsxapps/meteor/client/components/message/content/attachments/file/VideoAttachment.tsxapps/meteor/client/components/message/toolbar/useCopyAction.tsapps/meteor/client/components/message/toolbar/useReportMessageAction.tsxapps/meteor/client/hooks/useDecryptedMessage.spec.tsapps/meteor/client/hooks/useDecryptedMessage.tsapps/meteor/client/lib/chats/ChatAPI.tsapps/meteor/client/lib/chats/Upload.tsapps/meteor/client/lib/chats/flows/processMessageUploads.tsapps/meteor/client/lib/chats/uploads.tsapps/meteor/client/lib/normalizeThreadMessage.tsxapps/meteor/client/lib/parseMessageTextToAstMarkdown.spec.tsapps/meteor/client/lib/parseMessageTextToAstMarkdown.tsapps/meteor/client/lib/utils/normalizeMessagePreview/normalizeMessagePreview.spec.tsapps/meteor/client/lib/utils/normalizeMessagePreview/normalizeMessagePreview.tsapps/meteor/client/views/room/MessageList/hooks/useMessageBody.tsxapps/meteor/client/views/room/composer/messageBox/MessageComposerFileItem.tsxapps/meteor/client/views/room/composer/messageBox/MessageComposerFiles.tsxapps/meteor/client/views/room/composer/messageBox/MessageComposerGenericFile.tsxapps/meteor/client/views/room/contextualBar/ExportMessages/useDownloadExportMutation.tsapps/meteor/client/views/room/contextualBar/ExportMessages/useExportMessagesAsPDFMutation.tsxapps/meteor/client/views/room/contextualBar/Threads/hooks/useNormalizedThreadTitleHtml.tsapps/meteor/client/views/room/modals/FileUploadModal/FilePreview.tsxapps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsxapps/meteor/client/views/room/modals/FileUploadModal/ImagePreview.tsxapps/meteor/client/views/room/modals/FileUploadModal/MediaPreview.tsxapps/meteor/server/features/EmailInbox/EmailInbox_Outgoing.tsapps/meteor/server/services/messages/hooks/BeforeSaveMarkdownParser.tsapps/meteor/tests/end-to-end/api/rooms.tsapps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.tsapps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveMarkdownParser.tests.tsee/packages/federation-matrix/tests/end-to-end/messaging.spec.tsee/packages/omnichannel-services/src/OmnichannelTranscript.tspackages/core-typings/src/IMessage/MessageAttachment/Files/ImageAttachmentProps.tspackages/core-typings/src/IMessage/MessageAttachment/MessageAttachmentBase.tspackages/core-typings/src/IMessage/MessageAttachment/MessageAttachmentDefault.ts
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
dougfabris
force-pushed
the
refactor/restore-description-render-wa17
branch
from
6a164fc to
e8d14b5
Compare
Contributor
|
This looks more like a feature IMO, because you're fixing something, yes, but we add a new capability. Something like |
Member
Author
|
@nazabucciarelli we already added the capability, its just using a entry that shouldn't.. that's why is a fix |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40839 +/- ##
===========================================
+ Coverage 69.85% 69.89% +0.04%
===========================================
Files 3349 3349
Lines 124373 124370 -3
Branches 22163 22260 +97
===========================================
+ Hits 86878 86927 +49
+ Misses 34145 34091 -54
- Partials 3350 3352 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
Okay then we should update the PR title to describe 'what' is being fixed instead of the 'how' |
MartinSchoeler
previously approved these changes
Jun 10, 2026
dougfabris
changed the title
description being used as alternative text for images
Contributor
|
I think it's out of scope, but is it expected that when we try to get rid of the alternative text from the box, we just can't because the previous value shows up again? I think that if it's not a required field, we should allow the emptiness of its input 
|
Member
Author
|
@nazabucciarelli maybe you found a bug, let me check! |
nazabucciarelli
previously approved these changes
Jun 10, 2026
dougfabris
dismissed stale reviews from nazabucciarelli and MartinSchoeler
via
377cf39
ggazzo
previously approved these changes
Jun 10, 2026
dougfabris
force-pushed
the
refactor/restore-description-render-wa17
branch
from
377cf39 to
55a9756
Compare
nazabucciarelli
approved these changes
Jun 10, 2026
tassoevan
approved these changes
Jun 11, 2026
Member
|
/backport 8.5.1 |
Contributor
|
Sorry, I couldn't do that backport because of conflicts. Could you please solve them? you can do so by running the following commands: after that just run |
Member
Author
|
/backport 8.5.1 |
Contributor
|
Pull request #40938 added to Project: "Patch 8.5.1" |
Member
Author
|
/backport 8.4.4 |
Contributor
|
Sorry, I couldn't do that backport because of conflicts. Could you please solve them? you can do so by running the following commands: after that just run |
dougfabris
added a commit
that referenced
this pull request
Jun 12, 2026
Member
Author
|
/backport 8.4.4 |
Contributor
|
Pull request #40939 added to Project: "Patch 8.4.4" |
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
6 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.
Proposed changes (including videos or screenshots)
Introduces a dedicated
image_altfield on image attachments to hold accessibility alternative text, separating it fromdescriptionPreviously the "Alternative text" input from the upload modal was stored in
descriptionand used both as the<img alt>and as a visible caption for old messages. Splitting the concerns keeps alt text accessibility-only and freesdescriptionto render old messages.How to test the changes
<img>uses it asalt(inspect element / screen reader)Issue(s)
Steps to test or reproduce
Further comments
SUP-1039
Summary by CodeRabbit
New Features
Updates
Tests