fix: ContextualBar state and refresh behavior on custom sounds#38442
fix: ContextualBar state and refresh behavior on custom sounds#38442kodiakhq[bot] merged 14 commits intodevelopfrom
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: f92ff82 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThread a mandatory Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant UI as Client (Sidebar)
participant API as Server API
participant DB as Database
User->>UI: open sound for edit (select _id)
UI->>API: GET /custom-sounds?_id=<id>
API->>DB: find { _id: <id>, ...filters }
DB-->>API: sound data
API-->>UI: sound data
User->>UI: click Save
UI->>API: POST/PUT /custom-sounds (payload)
API->>DB: insert/update sound
DB-->>API: result
API-->>UI: success
UI->>UI: await save completion, call onChange to refresh list
UI->>UI: call close() to dismiss sidebar
UI-->>User: sidebar closed, list refreshed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (1)
33-72:⚠️ Potential issue | 🟡 MinorAdd
closetosaveActiondependency array and remove inline// FIXMEcomment.
closeis a prop captured within the callback but missing from the dependency list, which can cause stale closure bugs if the prop reference changes. The inline// FIXMEcomment violates the no-comments-in-implementation guideline and should be removed.🛠️ Proposed update
const saveAction = useCallback( - // FIXME async (name: string, soundFile: any) => { const soundData = createSoundData(soundFile, name); const validation = validate(soundData, soundFile) as Array<Parameters<typeof t>[0]>; validation.forEach((invalidFieldName) => { throw new Error(t('Required_field', { field: t(invalidFieldName) })); }); try { const soundId = await insertOrUpdateSound(soundData); if (!soundId) { return undefined; } dispatchToastMessage({ type: 'success', message: t('Uploading_file') }); const reader = new FileReader(); reader.readAsBinaryString(soundFile); reader.onloadend = (): void => { try { uploadCustomSound(reader.result as string, soundFile.type, { ...soundData, _id: soundId, random: Math.round(Math.random() * 1000), }); dispatchToastMessage({ type: 'success', message: t('File_uploaded') }); } catch (error) { (typeof error === 'string' || error instanceof Error) && dispatchToastMessage({ type: 'error', message: error }); } }; close(); return soundId; } catch (error) { (typeof error === 'string' || error instanceof Error) && dispatchToastMessage({ type: 'error', message: error }); } }, - [dispatchToastMessage, insertOrUpdateSound, t, uploadCustomSound], + [close, dispatchToastMessage, insertOrUpdateSound, t, uploadCustomSound], );apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
54-96:⚠️ Potential issue | 🟠 MajorGate sidebar close behind successful save; early return on validation errors, and add
closeto dependencies.
close()currently executes unconditionally at line 94, closing the sidebar even when validation fails. Restructure with an early return on validation errors and add the missingclosedependency to the hook. Remove the// FIXMEcomment per coding guidelines.🛠️ Proposed update
const saveAction = useCallback( - // FIXME async (sound: any) => { const soundData = createSoundData(sound, name, { previousName, previousSound, _id, extension: sound.extension }); const validation = validate(soundData, sound); - if (validation.length === 0) { - let soundId: string; - try { - soundId = await insertOrUpdateSound(soundData); - } catch (error) { - dispatchToastMessage({ type: 'error', message: error }); - return; - } - - soundData._id = soundId; - soundData.random = Math.round(Math.random() * 1000); - - if (sound && sound !== previousSound) { - dispatchToastMessage({ type: 'success', message: t('Uploading_file') }); - - const reader = new FileReader(); - reader.readAsBinaryString(sound); - reader.onloadend = (): void => { - try { - uploadCustomSound(reader.result as string, sound.type, { ...soundData, _id: soundId }); - return dispatchToastMessage({ type: 'success', message: t('File_uploaded') }); - } catch (error) { - dispatchToastMessage({ type: 'error', message: error }); - } - }; - } - } - - validation.forEach((invalidFieldName) => - dispatchToastMessage({ - type: 'error', - message: t('Required_field', { field: t(invalidFieldName) }), - }), - ); - - close(); + if (validation.length > 0) { + validation.forEach((invalidFieldName) => + dispatchToastMessage({ + type: 'error', + message: t('Required_field', { field: t(invalidFieldName) }), + }), + ); + return; + } + + let soundId: string; + try { + soundId = await insertOrUpdateSound(soundData); + } catch (error) { + dispatchToastMessage({ type: 'error', message: error }); + return; + } + + soundData._id = soundId; + soundData.random = Math.round(Math.random() * 1000); + + if (sound && sound !== previousSound) { + dispatchToastMessage({ type: 'success', message: t('Uploading_file') }); + + const reader = new FileReader(); + reader.readAsBinaryString(sound); + reader.onloadend = (): void => { + try { + uploadCustomSound(reader.result as string, sound.type, { ...soundData, _id: soundId }); + return dispatchToastMessage({ type: 'success', message: t('File_uploaded') }); + } catch (error) { + dispatchToastMessage({ type: 'error', message: error }); + } + }; + } + + close(); }, - [_id, dispatchToastMessage, insertOrUpdateSound, name, previousName, previousSound, t, uploadCustomSound], + [_id, close, dispatchToastMessage, insertOrUpdateSound, name, previousName, previousSound, t, uploadCustomSound], );
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38442 +/- ##
===========================================
- Coverage 70.41% 70.34% -0.07%
===========================================
Files 3162 3162
Lines 110663 110705 +42
Branches 19940 19869 -71
===========================================
- Hits 77923 77880 -43
- Misses 30711 30803 +92
+ Partials 2029 2022 -7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "02/10 20:06 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Tue, 10 Feb 2026 20:06:03 GMT |
nazabucciarelli
force-pushed
the
fix/custom-sounds-bug
branch
from
99be95b to
7514360
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/admin/customSounds/EditCustomSound.tsx (1)
19-29:⚠️ Potential issue | 🟡 MinorGuard the query when
_idis undefined to prevent unfiltered results during routing transitions.Although
EditCustomSoundis only rendered whencontext === 'edit', theidroute parameter fromuseRouteParameter('id')can be undefined if the route is accessed directly without the parameter or during routing transitions. This causesgetSounds({ _id: undefined })to execute, which may return all sounds instead of filtering. Addenabled: Boolean(_id)to gate the query and include a defensive check inqueryFn.♻️ Suggested guard
const { data, isPending } = useQuery({ queryKey: ['custom-sounds', _id], + enabled: Boolean(_id), queryFn: async () => { + if (!_id) { + throw new Error(t('No_results_found')); + } const { sounds } = await getSounds({ _id });
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/custom-sounds.ts (1)
58-61: Optional: keep the response schema tight unless you plan to return top‑level_id.
Since the response body doesn’t currently include a top‑level_id, consider dropping it from the schema (or add it to the response) to avoid confusing API consumers.♻️ Remove top‑level `_id` from response schema (if not intended)
- _id: { - type: 'string', - description: 'The ID of the sound.', - },
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
54-96:⚠️ Potential issue | 🟠 MajorMissing
closeinsaveActiondependency array.The
closefunction is called at line 85 but is not included in the dependency array at line 95. This can cause stale closure issues where an outdatedclosereference is captured.🐛 Proposed fix
- [_id, dispatchToastMessage, insertOrUpdateSound, name, previousName, previousSound, t, uploadCustomSound], + [_id, close, dispatchToastMessage, insertOrUpdateSound, name, previousName, previousSound, t, uploadCustomSound],
🤖 Fix all issues with AI agents
In `@apps/meteor/tests/end-to-end/api/custom-sounds.ts`:
- Around line 130-146: The test description string for the case inside the test
block (the it(...) call in apps/meteor/tests/end-to-end/api/custom-sounds.ts)
contains a typo ("requestes"); update that string to "requested" so it reads
"should return the requested custom sound when using the `_id` parameter". Only
change the human-readable test description in the it(...) call — do not modify
assertions, variable names like fileId, or the request flow.
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
112-112: Optional chaining is now unnecessary.Since
closeis now a required prop (line 12), the optional chainingclose?.()can be simplified toclose()for consistency with the usage at line 85 and line 153.♻️ Proposed fix
- close?.(); + close();
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/tests/end-to-end/api/custom-sounds.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/custom-sounds.ts:130">
P3: Typo in test description: "requestes" should be "requested".</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
aleksandernsilva
left a comment
aleksandernsilva
left a comment
There was a problem hiding this comment.
LGTM
PS.: There's definitely an opportunity to improve the UX and code style for this flow, but it is outside the scope of this PR.
nazabucciarelli
changed the title
Co-authored-by: Júlia Jaeger Foresti <60678893+juliajforesti@users.noreply.github.com>
aleksandernsilva
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dionisio-bot
bot
removed
the
stat: ready to merge
…custom-sounds-bug
…Chat into fix/custom-sounds-bug
nazabucciarelli
dismissed stale reviews from ricardogarim, juliajforesti, and aleksandernsilva
via
f92ff82
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/tests/end-to-end/api/custom-sounds.ts (1)
66-78:⚠️ Potential issue | 🟡 Minor
fileId2is never cleaned up in theafterhook.The
afterblock only deletesfileId, leaving the second custom sound (fileId2) orphaned after the test suite runs. Consider deleting both.Proposed fix
after(() => - request + Promise.all([request .post(api('method.call/deleteCustomSound')) .set(credentials) .send({ message: JSON.stringify({ msg: 'method', id: '33', method: 'deleteCustomSound', params: [fileId], }), - }), + }), + request + .post(api('method.call/deleteCustomSound')) + .set(credentials) + .send({ + message: JSON.stringify({ + msg: 'method', + id: '34', + method: 'deleteCustomSound', + params: [fileId2], + }), + }), + ]), );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/tests/end-to-end/api/custom-sounds.ts
🧰 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/tests/end-to-end/api/custom-sounds.ts
🧠 Learnings (18)
📚 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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 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/tests/end-to-end/api/custom-sounds.ts
📚 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/tests/end-to-end/api/custom-sounds.ts
📚 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/tests/end-to-end/api/custom-sounds.ts
📚 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/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 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/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 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/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2026-01-16T22:56:30.299Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 38224
File: apps/meteor/client/views/admin/viewLogs/AnalyticsReports.tsx:15-20
Timestamp: 2026-01-16T22:56:30.299Z
Learning: For the logs documentation link in apps/meteor/client/lib/links.ts, the correct URL slug is `/i/logs-docs` (plural), not `/i/logs-doc` (singular).
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 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 : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2026-02-04T12:09:05.769Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38374
File: apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.ts:26-37
Timestamp: 2026-02-04T12:09:05.769Z
Learning: In E2E tests at apps/meteor/tests/end-to-end/apps/, prefer sleeping for a fixed duration (e.g., 1 second) over implementing polling/retry logic when waiting for asynchronous operations to complete. Tests should fail deterministically if the expected result isn't available after the sleep.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 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 `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 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.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 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/tests/end-to-end/api/custom-sounds.ts
📚 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/tests/end-to-end/api/custom-sounds.ts
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/custom-sounds.ts (1)
packages/core-services/src/index.ts (1)
api(55-55)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/tests/end-to-end/api/custom-sounds.ts (1)
130-144: Clean test — LGTM on logic and assertions.The new test correctly verifies the
_idquery parameter, usesasync/await(preferable to thedonecallback pattern), and has appropriate assertions. The past typo has been fixed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
KevLehman
added
the
stat: ready to merge
|
when a PR fixes something but changes an API, a changeset for that API should be created. not 100% sure about a list being filtered by a single id |
6 participants
Proposed changes (including videos or screenshots)
These small changes fix the three problems described on the task ticket:
return sounds[0]line) because it seems that at some point in the past, the_idquery parameter was being taken in the backend and returned an array with a single value. Modifying thecustom-sounds.listbackend endpoint to receive and handle the_idquery parameter corrected the behavior. Furthermore, therefetch?.()line was removed fromhandleChange()since it's unneeded to refetch the edited Custom Sound after updating it because the list itself is updated upstream.close()after adding a new Custom Sounds to improve UX.Additionally to the above mentioned, an end-to-end test will be added to ensure the
_idquery parameter in thecustom-sounds.listendpoint is properly managed and this will not be removed in the future, breaking this feature again.Issue(s)
CORE-1789 Bugs when trying to edit custom sounds
Steps to test or reproduce
Further comments
On the ticket's comments you can observe a video of the behavior after the made modifications.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests
Chores
also closes: ARCH-1542