refactor: replace PermissionsTableFilter with FilterByText - #41186
Open
VanshajPoonia wants to merge 5 commits into
Open
refactor: replace PermissionsTableFilter with FilterByText#41186VanshajPoonia wants to merge 5 commits into
VanshajPoonia wants to merge 5 commits into
Conversation
Resolves the TODO left in RocketChat#41143. FilterByText renders the same search input, so the custom filter component is no longer needed. The filter state and its 500ms debounce now live in PermissionsTable. Closes RocketChat#41165
Contributor
|
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 |
🦋 Changeset detectedLatest commit: 4c1aa6c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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)
✅ Files skipped from review due to trivial changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
WalkthroughPermissionsTable now keeps local debounced text filter state, renders ChangesPermissions filter consolidation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.tsx`:
- Around line 26-31: The filter update in PermissionsTable currently only calls
setFilter(debouncedText), which leaves usePermissionsAndRoles on whatever
paginationData.current was previously selected. Update the existing useEffect
tied to debouncedText so that changing the debounced filter also resets the
current page to 0 before/along with applying the new filter, ensuring stale
offsets from later pages are cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3d98211-1619-4976-964e-e687a67023d7
⛔ 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 (2)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.tsxapps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTableFilter.tsx
💤 Files with no reviewable changes (1)
- apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTableFilter.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/PermissionsTable.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/PermissionsTable.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/PermissionsTable.tsx
🔇 Additional comments (1)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.tsx (1)
4-13: LGTM!Also applies to: 73-73
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Narrowing the filter while on a later page kept the stale offset, so the client-side slice in usePermissionsAndRoles could return an empty page and render the no-results state even though matches exist. Reset current to the first page whenever the debounced filter changes.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx (1)
100-122: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssertion doesn't actually verify the reset-on-filter-change behavior.
Per
PermissionsTable.tsx,setCurrent(0)fires unconditionally on every effect run, including the initial mount (debouncedTextstarts as''). SincesetCurrentis only ever called with0,expect(setCurrent).toHaveBeenLastCalledWith(0)would pass even if the reset-on-change logic were broken, as it's already satisfied by the mount-time call alone. Consider asserting call count (e.g.toHaveBeenCalledTimes(2)) or verifyingsetCurrentwas invoked again aftersetFilter('admin'), to actually exercise the commit's intended behavior.♻️ Suggested strengthening
await userEvent.type(screen.getByRole('textbox', { name: 'Search' }), 'admin'); await waitFor(() => expect(setFilter).toHaveBeenCalledWith('admin')); - expect(setCurrent).toHaveBeenLastCalledWith(0); + expect(setCurrent).toHaveBeenCalledTimes(2); + expect(setCurrent).toHaveBeenLastCalledWith(0);🤖 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/PermissionsTable.spec.tsx` around lines 100 - 122, The PermissionsTable pagination test is too weak because setCurrent(0) is already called on the initial mount, so the final assertion can pass even if the reset-on-filter-change behavior is broken. Update the PermissionsTable.spec.tsx test around the PermissionsTable render and userEvent.type flow to verify a second setCurrent call happens after the filter changes, either by asserting the call count increases or by checking the post-search invocation order alongside setFilter('admin').
🤖 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/PermissionsTable.spec.tsx`:
- Around line 100-122: The PermissionsTable pagination test is too weak because
setCurrent(0) is already called on the initial mount, so the final assertion can
pass even if the reset-on-filter-change behavior is broken. Update the
PermissionsTable.spec.tsx test around the PermissionsTable render and
userEvent.type flow to verify a second setCurrent call happens after the filter
changes, either by asserting the call count increases or by checking the
post-search invocation order alongside setFilter('admin').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db8892d4-d89e-4e83-aec4-04451f25ce9d
📒 Files selected for processing (2)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsxapps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/PermissionsTable.spec.tsx
🧠 Learnings (3)
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx
📚 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/PermissionsTable.spec.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/PermissionsTable.spec.tsx
🔇 Additional comments (1)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx (1)
4-4: LGTM!
Clear the setFilter/setCurrent mocks after mount so the assertion no longer passes on the mount-time setCurrent(0) call alone, and instead verifies the reset is triggered by the filter change itself.
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
1 participant
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)
Resolves the TODO left in #41143.
FilterByTextrenders the same search input, so the customPermissionsTableFiltercomponent is no longer needed.PermissionsTablenow renders the sharedFilterByTextcomponent directly and keeps the filter state plus the 500ms debounce, same wiring as the other admin tablesPermissionsTableFilter.tsxFilterByTextform wrapper and anaria-labelon the search inputNo user-facing behavior change, so no changeset added.
Issue(s)
Closes #41165
Steps to test or reproduce
Further comments
PermissionsTableprops are unchanged, soPermissionsPageis untouched. Lint, typecheck and thePermissionsTable.spec.tsxsuite (render, a11y and interaction tests) pass locally.Summary by CodeRabbit