regression: custom sound record breaks when validation error on update#40696
Merged
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
|
Contributor
WalkthroughThe update handler now upserts sound metadata before conditional file upload; a new E2E test verifies that a failed update (name collision) does not change metadata or the stored WAV file. ChangesSound Update Operation Safety
Sequence DiagramsequenceDiagram
participant Client
participant UpdateHandler
participant Database
participant FileStorage
Client->>UpdateHandler: PUT /custom-sounds.update (name, file)
UpdateHandler->>Database: insertOrUpdateSound (name, extension)
alt metadata update succeeds
Database-->>UpdateHandler: metadata persisted
UpdateHandler->>FileStorage: upload file (if present)
alt file upload succeeds
FileStorage-->>UpdateHandler: file stored
UpdateHandler-->>Client: 200 OK
else file upload fails
UpdateHandler-->>Client: error (metadata already saved)
end
else metadata update fails (validation error)
Database-->>UpdateHandler: error (e.g., name collision)
UpdateHandler-->>Client: 400 Bad Request (file not uploaded)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
nazabucciarelli
force-pushed
the
fix/custom-sounds-update
branch
from
9640aae to
ca8f097
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/api/server/v1/custom-sounds.ts (1)
262-275:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPre-upload metadata update can notify clients before the new file exists.
insertOrUpdateSound()persists the new name/extension and broadcasts immediately, butuploadCustomSound()writes${_id}.${extension}afterward. For rename + file replacement flows, subscribers can observe metadata that points to a file path that is still being written (or not written yet if the upload later fails). Consider splitting validation from persistence, or suppressing the pre-upload update/broadcast whenfileBufferis present.🤖 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/app/api/server/v1/custom-sounds.ts` around lines 262 - 275, The current flow calls insertOrUpdateSound(...) before uploadCustomSound(...), which broadcasts metadata (name/extension) that may reference a file not yet uploaded; change the logic so that when fileBuffer is present you do not persist/broadcast the final metadata up-front: instead perform validations (keep any validation logic), then call uploadCustomSound(fileBuffer, computedMimeType, {...}) first and only after a successful upload call insertOrUpdateSound(...) (or update the DB and broadcast) using the uploaded extension/_id; if you must persist earlier, suppress the broadcast in insertOrUpdateSound when fileBuffer is provided and emit an update after upload completes; reference insertOrUpdateSound, uploadCustomSound and soundToUpdate to implement this ordering or broadcast-suppression.
🤖 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/tests/end-to-end/api/custom-sounds.ts`:
- Around line 398-415: Add an assertion that the unexpected .mp3 path does not
exist by issuing a GET to `/custom-sounds/${soundAId}.mp3` (after the existing
checks for api('custom-sounds.getOne') and `/custom-sounds/${soundAId}.wav`) and
expecting a 404; reference the existing test variables soundAId/soundAName and
the same credentials to perform the request so the suite verifies no orphan
`${soundAId}.mp3` was created.
---
Outside diff comments:
In `@apps/meteor/app/api/server/v1/custom-sounds.ts`:
- Around line 262-275: The current flow calls insertOrUpdateSound(...) before
uploadCustomSound(...), which broadcasts metadata (name/extension) that may
reference a file not yet uploaded; change the logic so that when fileBuffer is
present you do not persist/broadcast the final metadata up-front: instead
perform validations (keep any validation logic), then call
uploadCustomSound(fileBuffer, computedMimeType, {...}) first and only after a
successful upload call insertOrUpdateSound(...) (or update the DB and broadcast)
using the uploaded extension/_id; if you must persist earlier, suppress the
broadcast in insertOrUpdateSound when fileBuffer is provided and emit an update
after upload completes; reference insertOrUpdateSound, uploadCustomSound and
soundToUpdate to implement this ordering or broadcast-suppression.
🪄 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: cc2b146b-d7df-4e20-8c8f-e5531a78b7b2
📒 Files selected for processing (2)
apps/meteor/app/api/server/v1/custom-sounds.tsapps/meteor/tests/end-to-end/api/custom-sounds.ts
📜 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)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- 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/tests/end-to-end/api/custom-sounds.tsapps/meteor/app/api/server/v1/custom-sounds.ts
🧠 Learnings (5)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 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/tests/end-to-end/api/custom-sounds.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/custom-sounds.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 2 files
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
KevLehman
reviewed
May 26, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/custom-sounds.ts (1)
378-379: 💤 Low valueConsider aligning cleanup patterns across all test suites in this file.
These lines remove the conditional guards present in other
afterhooks (e.g., lines 64–71, 76–80). While the "fail fast" approach can surface setup failures more clearly, the inconsistency may confuse future maintainers. Consider either adopting unconditional cleanup throughout the file or restoring the guards here for consistency.🤖 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/tests/end-to-end/api/custom-sounds.ts` around lines 378 - 379, The two cleanup calls await deleteCustomSound(soundAId) and await deleteCustomSound(soundBId) are unconditional while other after hooks use presence guards; update this block to match the file's pattern by either (A) wrapping both calls in conditional checks for soundAId and soundBId (e.g., if (soundAId) deleteCustomSound(soundAId)) to avoid throwing when setup failed, or (B) make all after hooks in this test file unconditional by removing guards elsewhere so cleanup is consistent; locate the after hook containing deleteCustomSound and apply the chosen consistent pattern across the file.
🤖 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/tests/end-to-end/api/custom-sounds.ts`:
- Around line 378-379: The two cleanup calls await deleteCustomSound(soundAId)
and await deleteCustomSound(soundBId) are unconditional while other after hooks
use presence guards; update this block to match the file's pattern by either (A)
wrapping both calls in conditional checks for soundAId and soundBId (e.g., if
(soundAId) deleteCustomSound(soundAId)) to avoid throwing when setup failed, or
(B) make all after hooks in this test file unconditional by removing guards
elsewhere so cleanup is consistent; locate the after hook containing
deleteCustomSound and apply the chosen consistent pattern across the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ea9ccff-0b29-4a1a-b003-ae09e162baab
📒 Files selected for processing (1)
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📜 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)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- 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/tests/end-to-end/api/custom-sounds.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 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/tests/end-to-end/api/custom-sounds.ts
🔇 Additional comments (2)
apps/meteor/tests/end-to-end/api/custom-sounds.ts (2)
2-2: LGTM!
382-418: LGTM!
KevLehman
approved these changes
May 26, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.5.0 #40696 +/- ##
================================================
Coverage ? 69.63%
================================================
Files ? 3338
Lines ? 123236
Branches ? 21968
================================================
Hits ? 85821
Misses ? 34059
Partials ? 3356
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
nazabucciarelli
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
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
2 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)
When an admin tried to update a custom sound but provided an invalid name (e.g., a name already in use), the update would correctly throw a 400 validation error, but the underlying audio file would break.
Swapped execution order: We now invoke
insertOrUpdateSoundfirst. If the payload fails validation (e.g., duplicate name), the error is thrown immediately, and the execution stops before any file manipulation occurs.Added Regression Test: Added an isolated integration test within the test suite to simulate a failed update (name collision with a different file extension) and assert that neither the database metadata nor the underlying file is mutated.
Issue(s)
CORE-2243 [Regression] Failed custom-sound rename can break the sound file
Steps to test or reproduce
Preconditions: Have two custom sounds, for example sound-a.mp3 and sound-b.mp3
e.g: replace the current .mp3 with an .wav
e.g: change it “sound-b”
Expected Behavior: The update should fail and leave sound-a unchanged and still play
Wrong Behavior: The update fails, and when you click to hear the sound-a it doesn’t play anymore. If the new file uses a different extension, the database still points to the old extension, and the sound becomes broken.
Further comments
Summary by CodeRabbit