fix: in app notification is not showing channel name and content#7046
fix: in app notification is not showing channel name and content#7046
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (2)
📜 Recent review details🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2026-03-10T15:21:38.113ZApplied to files:
🔇 Additional comments (4)
WalkthroughRemoved a flex: 1 layout constraint from NotifierComponent's inner View. Added Storybook stories and a snapshot test for the component. Adjusted jest.setup mock to merge real exports with mocked responsive-layout members. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
diegolmello
left a comment
There was a problem hiding this comment.
Can you write unit tests and stories?
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/containers/InAppNotification/NotifierComponent.stories.tsx`:
- Around line 46-75: The stories (DirectMessage, ChannelMessage, WithDarkTheme,
WithBlackTheme) render NotifierComponent without the required isMasterDetail
prop, producing an invalid prop shape for snapshot tests; update each story to
pass isMasterDetail explicitly (true or false depending on intended layout) when
rendering NotifierComponent so the component receives the correct prop shape and
snapshots reflect the correct handset vs master-detail layout.
In `@jest.setup.js`:
- Around line 94-107: The test mock for the responsive layout is returning a
partial context from useResponsiveLayout and omitting width and height, causing
destructuring in tests to see undefined; update the jest.mock for
'./app/lib/hooks/useResponsiveLayout/useResponsiveLayout' so the mocked
useResponsiveLayout includes width and height (e.g., sensible numeric defaults)
alongside fontScale, isLargeFontScale, fontScaleLimited, rowHeight,
rowHeightCondensed, and still export FONT_SCALE_LIMIT, ensuring tests receive
the full contract returned by the real module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c28fdd13-622a-42bb-b174-fb9cb9cb43b7
⛔ Files ignored due to path filters (1)
app/containers/InAppNotification/__snapshots__/NotifierComponent.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
app/containers/InAppNotification/NotifierComponent.stories.tsxapp/containers/InAppNotification/NotifierComponent.test.tsxjest.setup.js
📜 Review details
🔇 Additional comments (2)
app/containers/InAppNotification/NotifierComponent.test.tsx (1)
1-4: Nice coverage path for this regression.Reusing the Storybook fixtures here keeps the notifier variants and the snapshot test in sync, which is a good fit for this UI-only fix.
app/containers/InAppNotification/NotifierComponent.stories.tsx (1)
13-40: Good call making the stories self-contained.Providing
ThemeContextandResponsiveLayoutContextinside the story wrapper keeps these snapshots independent from global decorator setup.
Proposed changes
When we are using the app and receive a message in any channel, the app renders a blank area in the in-app notification
Regression introduced in #6997
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-1937
How to test or reproduce
Screenshots
Stories
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Style
Documentation
Tests