chore: fix jump to thread message e2e test#40690
Conversation
Co-authored-by: Douglas Gubert <douglas.gubert@gmail.com> Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
|
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 |
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesThread message jump and query parameter handling
Sequence Diagram(s)sequenceDiagram
participant ThreadMessageList
participant MessagesStore
participant Router
ThreadMessageList->>ThreadMessageList: scroll to msgJumpParam and set highlight
ThreadMessageList->>ThreadMessageList: setTimeout -> clearHighlightMessage() (2s)
ThreadMessageList->>MessagesStore: check for msg id existence (after 500ms)
MessagesStore-->>ThreadMessageList: message exists?
ThreadMessageList->>Router: clear `msg` query param (if exists and not mainMessage)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![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
🤖 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/client/views/room/contextualBar/Threads/components/ThreadMessageList.tsx`:
- Around line 138-149: The effect watching msgJumpParam currently depends on
items (an array recreated each render) which restarts the 500ms timeout
repeatedly; fix by making the effect depend on a stable identity instead of the
ephemeral items array: either memoize a derived stable value (e.g., itemsIds =
useMemo(() => items.map(m => m._id), [items.length, /*other stable keys*/]) and
use itemsIds in the dependency list) or store items in a ref (e.g.,
itemsRef.current = items) and read from itemsRef.current inside the useEffect
while keeping dependencies to [msgJumpParam, mainMessage._id]; update the effect
to check for the id via the stable value/ref and call
setMessageJumpQueryStringParameter(null) when found.
🪄 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: b44aee01-d039-41c4-80c2-41b058bf06ae
📒 Files selected for processing (1)
apps/meteor/client/views/room/contextualBar/Threads/components/ThreadMessageList.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). (5)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
🧰 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/contextualBar/Threads/components/ThreadMessageList.tsx
🧠 Learnings (2)
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/room/contextualBar/Threads/components/ThreadMessageList.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/client/views/room/contextualBar/Threads/components/ThreadMessageList.tsx
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## reg-unread #40690 +/- ##
==============================================
- Coverage 69.64% 69.64% -0.01%
==============================================
Files 3338 3338
Lines 123266 123266
Branches 21980 21931 -49
==============================================
- Hits 85847 85845 -2
- Misses 34057 34068 +11
+ Partials 3362 3353 -9
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}
There was a problem hiding this comment.
1 issue found across 1 file
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/views/room/contextualBar/Threads/components/ThreadMessageList.tsx">
<violation number="1" location="apps/meteor/client/views/room/contextualBar/Threads/components/ThreadMessageList.tsx:143">
P2: `msg` is no longer cleared when it points to the thread main message, which can leave the list stuck in deep-link mode and block normal auto-jump-to-bottom behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
1 participant
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit