fix: Sidebar message preview showing "undefined"#38262
fix: Sidebar message preview showing "undefined"#38262kodiakhq[bot] merged 8 commits intodevelopfrom
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: 9ce14ca The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 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 |
WalkthroughRename and consolidate message-preview utilities (normalizeNavigationMessage → normalizeMessagePreview, getNavigationMessagePreview → getMessagePreview), remove a legacy sidebar normalizer, update consumers to call the new utilities, and add Jest tests for both normalization and preview logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/meteor/client/lib/utils/normalizeMessagePreview/getMessagePreview.ts`:
- Around line 24-33: getMessagePreview currently interpolates sender labels
(lastMessage.u.name, lastMessage.u.username and t('You')) into HTML that may be
injected with dangerouslySetInnerHTML; escape or sanitize these labels before
concatenating to prevent XSS. In the getMessagePreview function, replace direct
interpolation of lastMessage.u.name/username and t('You') with an HTML-escaped
version (or return a structure that renders labels as plain text instead of
HTML) and ensure normalizedMessage remains the sanitized HTML payload; update
the branches that return `${t('You')}: ${normalizedMessage}` and
`${lastMessage.u.name || lastMessage.u.username}: ${normalizedMessage}` to use
the escapedLabel helper (or equivalent) so untrusted names cannot inject markup.
In
`@apps/meteor/client/views/navigation/sidepanel/omnichannel/InquireSidePanelItem.tsx`:
- Around line 31-32: The message preview can become the literal string
"undefined" because normalizeMessagePreview returns string | undefined; replace
its usage in InquireSidePanelItem.tsx by calling
getMessagePreview(room.lastMessage, t) when building the message variable
(currently using normalizeMessagePreview) and update imports to import
getMessagePreview instead of normalizeMessagePreview; ensure you keep the same
prefix logic (room.lastMessage && `${room.lastMessage.u.name ||
room.lastMessage.u.username}: ${...}`) so getMessagePreview supplies a safe
empty string rather than undefined.
🧹 Nitpick comments (1)
apps/meteor/client/lib/utils/normalizeMessagePreview/getMessagePreview.spec.ts (1)
126-152: Add a regression test for sender-name escaping.
Include a case wheremessage.u.namecontains HTML (e.g.,<img ...>), and assert the preview contains escaped output.
apps/meteor/client/lib/utils/normalizeMessagePreview/getMessagePreview.ts
Show resolved
Hide resolved
apps/meteor/client/views/navigation/sidepanel/omnichannel/InquireSidePanelItem.tsx
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38262 +/- ##
===========================================
- Coverage 70.44% 70.39% -0.06%
===========================================
Files 3162 3161 -1
Lines 110340 110639 +299
Branches 19841 19973 +132
===========================================
+ Hits 77726 77880 +154
- Misses 30581 30733 +152
+ Partials 2033 2026 -7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
dougfabris
left a comment
There was a problem hiding this comment.
I loved the refactor here, but we should split it from the fix
I decided to one shot this one because both places suffer from the same issue, and both the "refactored" functions were duplicates (one of which was outdated). If I fixed a single place, we'd still have the issue, if I had fixed both independently, it would look really silly dont you think? One of the places was having the same issue (showing "undefined") for encrypted messages, so I would also have had to copy the missing part from the other function. Seemed too counterproductive to me. |
Proposed changes (including videos or screenshots)
Issue(s)
CORE-1731
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Refactor
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.