fix: custom sounds pagination action buttons when the amount exceeds the specified limit#40113
Open
nazabucciarelli wants to merge 5 commits intodevelopfrom
Open
fix: custom sounds pagination action buttons when the amount exceeds the specified limit#40113nazabucciarelli wants to merge 5 commits intodevelopfrom
nazabucciarelli wants to merge 5 commits intodevelopfrom
Conversation
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: 578abd6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 as they are similar to previous changes (1)
📜 Recent review details⏰ 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). (4)
WalkthroughFixes admin Custom Sounds pagination by using the API-provided total count, adds a Storybook story and tests for the table, and introduces a changeset marking a patch release for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 @@
## develop #40113 +/- ##
===========================================
- Coverage 70.28% 70.24% -0.05%
===========================================
Files 3280 3283 +3
Lines 116814 117012 +198
Branches 20665 20739 +74
===========================================
+ Hits 82107 82192 +85
- Misses 31430 31536 +106
- Partials 3277 3284 +7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
juliajforesti
previously approved these changes
Apr 13, 2026
Contributor
juliajforesti
left a comment
juliajforesti
left a comment
There was a problem hiding this comment.
Although there is no test for this fix, I don't think it's price worth paying: it would require adding 25+ custom sounds just to test pagination, which is disproportionate to the fix's simplicity.
dougfabris
requested changes
Apr 13, 2026
Member
There was a problem hiding this comment.
Although there is no test for this fix, I don't think it's price worth paying: it would require adding 25+ custom sounds just to test pagination, which is disproportionate to the fix's simplicity.
Actually, a snapshot testing for this table is very welcome here!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx (1)
28-41: Click forward pagination button and assert second-page content loads; avoid ESLint suppression comments.The test currently verifies only that the forward button exists and is enabled (lines 36–38), without interacting with it or confirming that navigation actually changes page content. Also, the ESLint suppression comment on line 35 violates the coding guideline to avoid comments in implementation.
Interact with the forward button and assert that second-page content appears (for example, by finding
Custom Sound 26after clicking). This ensures pagination behaves correctly, not just that controls are present. If the forward button lacks a semantic accessible name, consider whether the Pagination component from@rocket.chat/fuselageneeds anaria-labelattribute or similar fix; the class-based selector is brittle and should be avoided in favor of semantic locators.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx` around lines 28 - 41, The test currently only checks the forward pagination control exists; update the test in CustomSoundsTable.spec.tsx to click the forward pagination button and assert page 2 content loads (e.g., use screen.getByRole or getByLabelText for the forward control instead of container.querySelector('.rcx-pagination__forward'), perform userEvent.click on that control, then await screen.findByText('Custom Sound 26') to confirm navigation). Remove the ESLint suppression comment and prefer semantic queries (getByRole/getByLabelText) for the Pagination control; if the Pagination component (used by the Default story from composeStories(stories)) lacks an accessible name, add an aria-label when rendering in the story or the test wrapper so you can select it via getByLabelText.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx`:
- Around line 28-41: The test currently only checks the forward pagination
control exists; update the test in CustomSoundsTable.spec.tsx to click the
forward pagination button and assert page 2 content loads (e.g., use
screen.getByRole or getByLabelText for the forward control instead of
container.querySelector('.rcx-pagination__forward'), perform userEvent.click on
that control, then await screen.findByText('Custom Sound 26') to confirm
navigation). Remove the ESLint suppression comment and prefer semantic queries
(getByRole/getByLabelText) for the Pagination control; if the Pagination
component (used by the Default story from composeStories(stories)) lacks an
accessible name, add an aria-label when rendering in the story or the test
wrapper so you can select it via getByLabelText.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 158a5aa3-3c54-4030-886f-ecd67de6a42b
⛔ Files ignored due to path filters (1)
apps/meteor/client/views/admin/customSounds/CustomSoundsTable/__snapshots__/CustomSoundsTable.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsxapps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.stories.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.stories.tsx
📜 Review details
⏰ 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🧰 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/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx
🧠 Learnings (12)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx
📚 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/customSounds/CustomSoundsTable/CustomSoundsTable.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/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx
🔇 Additional comments (2)
apps/meteor/client/views/admin/customSounds/CustomSoundsTable/CustomSoundsTable.spec.tsx (2)
15-21: Strong bug-repro mock setup for pagination state.This mock captures the exact regression condition (
countpage slice vstotaloverall), so it validates the intended fix path well.
23-26: Good story-level regression coverage.Parameterized snapshot coverage across composed stories is a solid safety net for UI regressions in this table.
nazabucciarelli
commented
Apr 13, 2026
| const firstSound = await screen.findByText('Custom Sound 1'); | ||
| expect(firstSound).toBeInTheDocument(); | ||
|
|
||
| // eslint-disable-next-line testing-library/no-container, testing-library/no-node-access |
Contributor
Author
There was a problem hiding this comment.
Note for reviewers: added this comment to suppress the warning of using node access instead of name access, since it's the only way to locate the element currently
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
3 participants
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)
The issue was that we were relying on
data.sounds.lengthto decide the pagination state, but this value represents the total of records in the page. It actually must depend ondata?.total, which represents the total of records.Behavior after the change:
Issue(s)
CORE-1827 Fix Custom Sounds pagination
Steps to test or reproduce
Login as admin and add more than 25 custom sounds (I suggest as a quick way running custom-sounds API tests many times with
afterblocks commented out, so no cleanup is done) and observe that you are able to move forwards or backwards with the pagination.Further comments
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
Chores