regression: Prevent PermissionsTableFilter from growing - #41143
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 |
|
WalkthroughA TODO comment was added to PermissionsTableFilter noting it should be replaced by FilterByText, and its returned TextInput element was wrapped in a div. ChangesPermissionsTableFilter Wrapper Update
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.6.0 #41143 +/- ##
================================================
Coverage ? 70.17%
================================================
Files ? 3371
Lines ? 130446
Branches ? 22659
================================================
Hits ? 91544
Misses ? 35583
Partials ? 3319
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTableFilter.tsx (1)
7-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix TODO grammar; also flagged by path guideline.
"Replaced this by FilterByText" should read "Replace this with FilterByText". As per path instructions,
**/*.{ts,tsx,js}files should "Avoid code comments in the implementation" — consider tracking this as a follow-up ticket/issue instead of an inline comment.✏️ Proposed fix
-/** - * - * TODO: Replaced this by FilterByText, it has the same render - */ +// TODO: Replace this with FilterByText, it has the same render🤖 Prompt for 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. In `@apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTableFilter.tsx` around lines 7 - 10, The TODO comment in PermissionsTableFilter is grammatically incorrect and should not remain as an inline implementation comment in this TypeScript/TSX file. Update the wording from “Replaced this by FilterByText” to “Replace this with FilterByText,” and since the path guideline discourages code comments here, remove the inline TODO entirely and capture the follow-up in a ticket or issue instead.Source: Path instructions
🤖 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.
Nitpick comments:
In
`@apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTableFilter.tsx`:
- Around line 7-10: The TODO comment in PermissionsTableFilter is grammatically
incorrect and should not remain as an inline implementation comment in this
TypeScript/TSX file. Update the wording from “Replaced this by FilterByText” to
“Replace this with FilterByText,” and since the path guideline discourages code
comments here, remove the inline TODO entirely and capture the follow-up in a
ticket or issue instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3a406e8-e063-422b-890b-0c4c9b83cbe9
⛔ Files ignored due to path filters (1)
apps/meteor/client/views/admin/permissions/PermissionsTable/__snapshots__/PermissionsTable.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTableFilter.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (2/4)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (5/5)
🧰 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/admin/permissions/PermissionsTable/PermissionsTableFilter.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/admin/permissions/PermissionsTable/PermissionsTableFilter.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/admin/permissions/PermissionsTable/PermissionsTableFilter.tsx
🔇 Additional comments (1)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTableFilter.tsx (1)
24-28: LGTM!
2 participants
Proposed changes (including videos or screenshots)
Introduced here: #41066
Wraps the search TextInput in the permissions table filter with a container div to prevent it from growing due to the changes we did on fuselage v0.79.1
before
after
Issue(s)
Steps to test or reproduce
Further comments
CORE-2372
Summary by CodeRabbit