fix: Client crash on missing proper attachments.fields
#37147
Conversation
|
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: a1d7f16 The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 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 |
WalkthroughThis PR fixes a client crash (TypeError) that occurred when rendering message attachment fields missing the 'title' property. The DefaultAttachment component now conditionally renders title and value fields as MarkdownText only when they exist, setting them to null otherwise. A changeset documents the patch version bump. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Simple, localized conditional rendering fix in a single component addressing a specific crash scenario. Changes are straightforward and low-risk. Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
⏰ 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). (7)
🔇 Additional comments (2)
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 #37147 +/- ##
===========================================
- Coverage 67.58% 67.55% -0.03%
===========================================
Files 3295 3295
Lines 112648 112652 +4
Branches 20455 20453 -2
===========================================
- Hits 76133 76106 -27
- Misses 33842 33871 +29
- Partials 2673 2675 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/chat.ts (1)
558-585: Add a success case for non-string field valuesOnce the validation bug is fixed, please extend the happy-path test to cover a field like
{ value: 0 }or{ value: false }. Those payloads were valid before and should remain so; adding a test here will keep us from regressing again.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/hungry-fans-wait.md(1 hunks)apps/meteor/app/lib/server/functions/sendMessage.ts(1 hunks)apps/meteor/client/components/message/content/attachments/DefaultAttachment.tsx(1 hunks)apps/meteor/tests/end-to-end/api/chat.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/chat.ts (1)
apps/meteor/tests/data/api-data.ts (3)
request(10-10)api(46-48)credentials(39-42)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
apps/meteor/client/components/message/content/attachments/DefaultAttachment.tsx
Show resolved
Hide resolved
|
why |
@dougfabris This PR introduces API validation on |
dougfabris
changed the title
title and value properties should be required on attachments.fieldstitle and value properties should be required on attachments.fields
|
@yash-rajpal : Since this is a breaking change, it must target the |
yash-rajpal
force-pushed
the
fix/attachement-fields
branch
from
942798c to
aa2a2ac
Compare
yash-rajpal
requested review from
MartinSchoeler and
aleksandernsilva
and removed request for
a team
dougfabris
changed the title
title and value properties should be required on attachments.fieldstitle and value properties should be required on attachments.fields
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/chilled-pans-unite.md (1)
5-5: Clarify the changeset message (imperative, specify missing keys).Be explicit about the condition and use imperative voice for consistency.
Apply this diff:
-Fixes client crash on invalid message attachment fields payload. +Fix client crash when rendering message attachment fields with missing title/value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/chilled-pans-unite.md(1 hunks)apps/meteor/client/components/message/content/attachments/DefaultAttachment.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/components/message/content/attachments/DefaultAttachment.tsx
🔇 Additional comments (1)
.changeset/chilled-pans-unite.md (1)
2-2: Confirm package scope for the bump.If the changes are confined to apps/meteor (DefaultAttachment.tsx), scoping to '@rocket.chat/meteor' is correct. If other packages consume or export the affected component or types, include them in this changeset too.
yash-rajpal
changed the title
title and value properties should be required on attachments.fieldsattachments.fields
aleksandernsilva
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Fixes a client crash caused by missing title or value properties in attachment fields.
The web client expects both title and value to always be present, but our APIs currently allow attachments without these parameters.
API validation will be added on release 8.0.0 since it is a breaking change - #37233
Issue(s)
Steps to test or reproduce
Further comments
SUP-854
No tests here since this will be properly fixed on release-8.0.0 by PR #37233 which contains appropriate tests.
Summary by CodeRabbit