feat: show @all and @here in mention list based on user permission#6821
feat: show @all and @here in mention list based on user permission#6821diegolmello merged 12 commits intodevelopfrom
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds two new permissions ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
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.
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.
All good, but I'm missing autocomplete tests
I have added the unit test :) |
There was a problem hiding this comment.
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 :) |
ee1d1d7 to
b1f9867
Compare
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.