regression: Message List flaky jump to bottom#40794
Merged
Merged
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
|
Contributor
|
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 selected for processing (1)
📜 Recent 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). (4)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (11)📓 Common learnings📚 Learning: 2026-06-02T13:27:22.143ZApplied to files:
📚 Learning: 2026-05-26T19:18:05.882ZApplied to files:
📚 Learning: 2026-04-28T14:08:46.920ZApplied to files:
📚 Learning: 2026-03-11T22:04:20.529ZApplied to files:
📚 Learning: 2026-04-29T20:06:34.862ZApplied to files:
📚 Learning: 2026-02-24T19:36:55.089ZApplied to files:
📚 Learning: 2026-04-18T12:32:53.425ZApplied to files:
📚 Learning: 2026-04-13T16:40:55.864ZApplied to files:
📚 Learning: 2026-03-27T14:52:56.865ZApplied to files:
📚 Learning: 2026-05-06T12:21:44.083ZApplied to files:
🔇 Additional comments (1)
Walkthrough
ChangesScroll state initialization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
10 issues found across 196 files
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/app/apple/server/loginHandler.ts">
<violation number="1">
P0: Removed `await` turns `result` into a Promise, so `result.userId` is undefined and the login handler can incorrectly return a cancellation error.</violation>
</file>
<file name="apps/meteor/app/apple/server/appleOauthRegisterService.ts">
<violation number="1">
P1: Apple OAuth private key newline normalization was removed, which can break JWT signing for keys stored with escaped `\n`.</violation>
</file>
<file name="apps/meteor/app/lib/server/methods/createToken.ts">
<violation number="1">
P1: Removing `await` from `_insertLoginToken` introduces an async ordering bug: token-limit enforcement and method return can happen before the login token is actually persisted.
(Based on your team's feedback about concurrency-sensitive sequencing changes in async flows.) [FEEDBACK_USED]</violation>
</file>
<file name=".changeset/pre.json">
<violation number="1">
P1: Adding `.changeset/pre.json` forces Changesets prerelease mode and changes release automation behavior for the repo. This is out of scope for a Message List regression fix and should be reverted from this PR.
(Based on your team's feedback about reverting out-of-scope generated release files.) [FEEDBACK_USED].</violation>
</file>
<file name="apps/meteor/app/drupal/server/lib.ts">
<violation number="1">
P1: The Drupal OAuth server URL is no longer sanitized/validated before configure, so invalid values can produce malformed token/identity endpoints and break OAuth login.</violation>
</file>
<file name="apps/meteor/app/dolphin/server/lib.ts">
<violation number="1">
P1: OAuth runtime config now watches only URL, so clientId/secret changes are not reapplied and can leave stale credentials in ServiceConfiguration.</violation>
</file>
<file name="apps/meteor/client/views/room/MessageList/hooks/useTryToJumpToMessage.ts">
<violation number="1">
P1: The new `tshow` exception makes MessageList handle some thread replies that are still unconditionally handled by `useTryToJumpToThreadMessage`, creating conflicting jump flows (main-list scroll vs thread navigation).
(Based on your team's feedback about avoiding duplicate msg-jump handling across MessageList and ThreadMessageList.) [FEEDBACK_USED].</violation>
</file>
<file name="apps/meteor/app/api/server/v1/custom-sounds.ts">
<violation number="1">
P1: The reordered update sequence can persist/broadcast the new sound metadata before the new file is successfully stored, leaving DB/file state inconsistent if upload fails.
(Based on your team's feedback about execution-order and race/concurrency implications in this flow.) [FEEDBACK_USED].</violation>
</file>
<file name="apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts">
<violation number="1">
P1: Returning the async jump handler from `streamNewMessage` makes callback failures propagate into the message stream pipeline, which can skip downstream message-side effects for that event. Fire-and-forget this jump action (with local error handling) instead of returning its Promise.
(Based on your team's feedback about concurrency/ordering changes in message flow.) [FEEDBACK_USED].</violation>
</file>
<file name="apps/meteor/app/gitlab/server/lib.ts">
<violation number="1">
P2: Mutable fallback uses previously overwritten OAuth values, so clearing GitLab URL/path settings cannot revert to defaults.</violation>
</file>
Note: This PR contains a large number of files. cubic only reviews up to 100 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.
Re-trigger cubic
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5.0 #40794 +/- ##
================================================
Coverage ? 69.91%
================================================
Files ? 3327
Lines ? 126590
Branches ? 22009
================================================
Hits ? 88501
Misses ? 34800
Partials ? 3289
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
gabriellsh
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Issue(s)
CORE-2216
Steps to test or reproduce
Further comments
Summary by CodeRabbit