-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: show @all and @here in mention list based on user permission #6821
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
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughAdds two new supported permissions, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/containers/MessageComposer/hooks/useAutocomplete.ts (1)
216-216: AddmentionAllandmentionHereto the dependency array.The
useEffecthook usesmentionAllandmentionHere(lines 94, 103) but doesn't include them in its dependency array. This violates the exhaustive-deps rule and could lead to stale closures where the effect uses outdated permission values.Apply this diff to fix the dependency array:
- }, [text, type, rid, commandParams]); + }, [text, type, rid, commandParams, mentionAll, mentionHere]);
📜 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 (2)
app/containers/MessageComposer/hooks/useAutocomplete.ts(4 hunks)app/lib/methods/getPermissions.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/containers/MessageComposer/hooks/useAutocomplete.ts (1)
app/lib/hooks/usePermissions.ts (1)
usePermissions(52-59)
⏰ 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: ESLint and Test / run-eslint-and-test
🔇 Additional comments (3)
app/containers/MessageComposer/hooks/useAutocomplete.ts (2)
19-19: LGTM!The import statement is correct and necessary for the permission checks.
94-111: LGTM!The conditional logic correctly guards the display of
@alland@herementions behind their respective permission checks and text matching criteria. Once theridparameter is passed tousePermissions(as noted in the previous comment), this implementation will function as intended.app/lib/methods/getPermissions.ts (1)
69-71: LGTM!The new permission entries for
mention-allandmention-hereare properly added to theSUPPORTED_PERMISSIONSarray. The formatting is consistent with the existing structure, and these permissions correctly support the autocomplete feature.
diegolmello
left a comment
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.
All good, but I'm missing autocomplete tests
I have added the unit test :) |
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: 1
📜 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 ignored due to path filters (1)
app/containers/MessageComposer/__snapshots__/MessageComposer.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
app/containers/MessageComposer/MessageComposer.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/containers/MessageComposer/MessageComposer.test.tsx (1)
app/reducers/mockedStore.ts (1)
mockedStore(7-7)
🔇 Additional comments (1)
app/containers/MessageComposer/MessageComposer.test.tsx (1)
442-504: Comprehensive permission test coverage.These tests effectively validate all four permission combinations for @ALL and @here mentions, ensuring the autocomplete behavior matches the PR requirements. The test structure follows existing patterns and includes appropriate assertions for both presence and absence of autocomplete items.
|
Requesting a review :) |
Proposed changes
Right now we show @ALL and @here to all user even they don't have permission to use them. So with this PR, we will just check user permission and show @ALL or @here based on the permission.
Issue(s)
https://rocketchat.atlassian.net/browse/COMM-80
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.