-
Notifications
You must be signed in to change notification settings - Fork 13k
fix(federation): mentions being shown as plain text on RC side #37140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 |
|
WalkthroughServer and client mention handling were normalized: server now trims or strips mention prefixes conditionally; client-side mention resolution and composer popup normalize usernames by removing a leading Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant C as Client (Composer/Renderer)
participant R as Resolver (GazzodownText)
participant S as Server (Mentions)
U->>C: Type/select @mention
C->>C: Normalize selected value (strip leading "@")
C->>R: Resolve mention token
R->>R: Normalize mention (strip "@")\nFilter out entries with missing username or type 'team'\nCompare normalized usernames
R-->>C: Resolved mention metadata
C->>S: Send message with mention tokens
S->>S: Parse mentions:\n- If token contains ':' -> keep trimmed\n- Else -> trim and substring(1)
S-->>C: Parsed mentions acknowledged
C-->>U: Render message with resolved mentions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (3)
🔇 Additional comments (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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37140 +/- ##
==================================================
- Coverage 66.36% 66.35% -0.02%
==================================================
Files 3386 3386
Lines 115661 115666 +5
Branches 21362 21360 -2
==================================================
- Hits 76761 76746 -15
- Misses 36295 36314 +19
- Partials 2605 2606 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
a6fae0b to
a46c7a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/mentions/server/Mentions.ts (1)
57-57: Clarify federated mention logic.
Add a comment at line 57 noting thatm.includes(':')detects federated usernames (e.g.@user:server) which must be kept intact, while other mentions strip the leading@. Thesubstring(1)call is safe since the regex ensures non-empty matches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/app/mentions/server/Mentions.ts(2 hunks)apps/meteor/client/components/GazzodownText.tsx(1 hunks)apps/meteor/client/views/room/providers/ComposerPopupProvider.tsx(1 hunks)
⏰ 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). (7)
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/client/views/room/providers/ComposerPopupProvider.tsx (1)
156-156: Username normalization consistencyComposerPopupProvider.tsx now strips leading ‘@’ from
item.usernamejust like in GazzodownText.tsx; no othergetValue(username)patterns were found. Verify this covers all mention scenarios (e.g., federated usernames).apps/meteor/client/components/GazzodownText.tsx (1)
69-74: Verify team mention normalization
Confirm whetherUserMention.nameincludes a leading “@”. If it doesn’t, either strip “@” frommentioninfilterTeamor normalizenamebefore comparison to ensure team mentions match correctly.apps/meteor/app/mentions/server/Mentions.ts (1)
82-82: Approve prefix stripping for channel mentions. The.substring(1)call correctly removes the#prefix for both local (#general) and federated (#room@server) mentions.
a46c7a4 to
9e9dcbd
Compare
6fc8f40 to
928973e
Compare
89ebe54 to
94445d8
Compare
9e9dcbd to
29ed515
Compare
29ed515 to
e30344a
Compare
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Chores