regression: Prevent composer recreation on viewport resize#39881
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughThe MessageBox component's autofocus behavior now relies on a media query detecting touch capability ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.3.0 #39881 +/- ##
=================================================
- Coverage 70.56% 70.56% -0.01%
=================================================
Files 3256 3256
Lines 115781 115782 +1
Branches 20970 21040 +70
=================================================
- Hits 81700 81697 -3
- Misses 32023 32030 +7
+ Partials 2058 2055 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
0a7dfb1 to
348d503
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx (1)
145-146: Please add a regression test for attachment persistence on resize.This fixes a user-facing state-loss bug; adding coverage for “attach file → resize across md breakpoint → attachment still present” would prevent recurrence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx` around lines 145 - 146, Add a regression test that reproduces “attach file → resize across md breakpoint → attachment still present” for the MessageBox component: in the test for MessageBox (the component that uses useMessageBoxAutoFocus and computes isTouchDevice via useMediaQuery), simulate attaching a file (e.g., fire the file input change or call the attach handler), then programmatically change the viewport size to cross the md breakpoint (to flip useMediaQuery('(pointer: coarse)') result), and assert the attachment state still exists in the DOM/state after the resize; ensure the test mounts the same component tree that uses useMessageBoxAutoFocus so the focus/resize interaction is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx`:
- Around line 145-146: Add a regression test that reproduces “attach file →
resize across md breakpoint → attachment still present” for the MessageBox
component: in the test for MessageBox (the component that uses
useMessageBoxAutoFocus and computes isTouchDevice via useMediaQuery), simulate
attaching a file (e.g., fire the file input change or call the attach handler),
then programmatically change the viewport size to cross the md breakpoint (to
flip useMediaQuery('(pointer: coarse)') result), and assert the attachment state
still exists in the DOM/state after the resize; ensure the test mounts the same
component tree that uses useMessageBoxAutoFocus so the focus/resize interaction
is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf3735eb-7a33-43c4-b1a3-cbf9b0fdd543
📒 Files selected for processing (1)
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx
📜 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx
🧠 Learnings (7)
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx
📚 Learning: 2026-03-18T16:08:17.800Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39590
File: apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx:97-99
Timestamp: 2026-03-18T16:08:17.800Z
Learning: In `apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx`, `reValidateMode: 'onBlur'` is intentionally used (not 'onChange') because the `validateEmailFormat` and `validatePhone` functions are async and call the `checkExistenceEndpoint` API to check for duplicates. Using 'onChange' would trigger excessive network requests on every keystroke. The combination of `mode: 'onSubmit'` with `reValidateMode: 'onBlur'` is a deliberate design decision to minimize API calls while still providing revalidation feedback.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx
🔇 Additional comments (2)
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx (2)
3-3: Import change is correct and minimally scoped.
useMediaQueryis introduced exactly where it is needed, with no extra import churn.
145-146: Autofocus gating is now stable across breakpoint resize.Using pointer capability instead of
isMobileshould keep the autofocus ref callback from changing when the viewport crosses 768px, which directly addresses the composer recreation path.
Proposed changes (including videos or screenshots)
Resizing the browser across the 768px breakpoint destroys and recreates the composer, losing the uploads store state.
Replace the
isMobileprop inuseMessageBoxAutoFocuswith a(pointer: coarse)media query, making the ref callback stable and preventing unnecessary composer recreation on resize.This issue wasn't introduce in #39425 but it became easier to identify after it
Issue(s)
Steps to test or reproduce
Further comments
CORE-2007
Summary by CodeRabbit