refactor(core-typings): Record types missing a required _updatedAt field#38528
refactor(core-typings): Record types missing a required _updatedAt field#38528
_updatedAt field#38528Conversation
|
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 |
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughConsolidates record identity/timestamps under IRocketChatRecord, narrows input types to exclude Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Server as CustomSounds Service
participant DB as Database
participant Notify as Notify/Broadcast
Note over Client,Server: Upload or rename request initiated
Client->>Server: upload sound / rename request
Server->>DB: write (insert / findOneAndUpdate)
DB-->>Server: returns inserted/updated document (or null)
alt document returned
Server->>Notify: broadcast `updateCustomSound` with full sound object
Notify->>Client: notify clients
else no document
Server-->>Client: no broadcast
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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. ✨ Finishing touches🧪 Generate unit tests (beta)
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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38528 +/- ##
===========================================
- Coverage 70.39% 70.38% -0.01%
===========================================
Files 3161 3161
Lines 110654 110653 -1
Branches 19892 19849 -43
===========================================
- Hits 77895 77884 -11
- Misses 30731 30742 +11
+ Partials 2028 2027 -1
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/06 19:47 (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: Fri, 06 Feb 2026 19:47:14 GMT |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts`:
- Around line 32-40: The ws.on('end') handler uses an async setTimeout with a
fixed 500ms delay which is a race condition and creates unhandled promise
rejections; change this to avoid the magic timeout and to handle errors: either
(preferred) perform an atomic read-after-write (e.g., use the same insert/update
operation or a findOneAndUpdate on CustomSounds to get the final document and
broadcast immediately from that promise), or (minimal) replace the async
setTimeout with a proper promise chain that awaits CustomSounds.findOneById and
calls api.broadcast inside a try/catch so rejections are handled before calling
resolve; remove the fire-and-forget async callback to eliminate unhandled
promise rejections and reference ws.on('end'), setTimeout,
CustomSounds.findOneById and api.broadcast when making the change.
🧹 Nitpick comments (1)
apps/meteor/tests/data/livechat/department.ts (1)
200-205:delete department._updatedAtis now contradictory with the parameter type.The parameter is typed as
Omit<ILivechatDepartment, '_updatedAt'>, signaling_updatedAtshouldn't be present, yet line 202 still deletes it. This only compiles becauseILivechatDepartmenthas an index signature ([k: string]: any). Consider removing thedeletestatement since the type contract already guarantees the field isn't there, or keep the originalILivechatDepartmenttype if the intent is to defensively strip it at runtime.Option A: Remove the dead delete (trust the type)
export const disableDepartment = async (department: Omit<ILivechatDepartment, '_updatedAt'>): Promise<void> => { department.enabled = false; - delete department._updatedAt; const updatedDepartment = await updateDepartment(department._id, department); expect(updatedDepartment.enabled).to.be.false; };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/meteor/app/custom-sounds/server/methods/insertOrUpdateSound.tsapps/meteor/app/custom-sounds/server/methods/uploadCustomSound.tsapps/meteor/app/livechat/server/business-hour/LivechatBusinessHours.tsapps/meteor/app/livechat/server/business-hour/filterBusinessHoursThatMustBeOpened.tsapps/meteor/client/lib/userStatuses.tsapps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsxapps/meteor/client/providers/CustomSoundProvider/lib/helpers.tsapps/meteor/client/views/admin/workspace/DeploymentCard/components/InstancesModal/InstancesModal.stories.tsxapps/meteor/client/views/admin/workspace/DeploymentCard/components/InstancesModal/InstancesModal.tsxapps/meteor/tests/data/livechat/department.tspackages/core-services/src/events/Events.tspackages/core-typings/src/ICustomSound.tspackages/core-typings/src/ICustomUserStatus.tspackages/core-typings/src/IInquiry.tspackages/core-typings/src/IInstanceStatus.tspackages/core-typings/src/ILivechatBusinessHour.tspackages/core-typings/src/ILivechatDepartment.tspackages/model-typings/src/models/ICustomSoundsModel.tspackages/model-typings/src/models/ILivechatBusinessHoursModel.tspackages/models/src/models/CustomSounds.tspackages/models/src/models/LivechatBusinessHours.tspackages/ui-contexts/src/CustomSoundContext.ts
💤 Files with no reviewable changes (3)
- apps/meteor/client/views/admin/workspace/DeploymentCard/components/InstancesModal/InstancesModal.stories.tsx
- packages/core-typings/src/IInquiry.ts
- apps/meteor/client/views/admin/workspace/DeploymentCard/components/InstancesModal/InstancesModal.tsx
🧰 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:
packages/core-services/src/events/Events.tsapps/meteor/app/livechat/server/business-hour/LivechatBusinessHours.tsapps/meteor/app/livechat/server/business-hour/filterBusinessHoursThatMustBeOpened.tspackages/core-typings/src/ILivechatBusinessHour.tspackages/core-typings/src/IInstanceStatus.tsapps/meteor/app/custom-sounds/server/methods/insertOrUpdateSound.tspackages/core-typings/src/ICustomUserStatus.tsapps/meteor/tests/data/livechat/department.tspackages/model-typings/src/models/ILivechatBusinessHoursModel.tspackages/model-typings/src/models/ICustomSoundsModel.tspackages/models/src/models/LivechatBusinessHours.tspackages/core-typings/src/ILivechatDepartment.tsapps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsxapps/meteor/client/providers/CustomSoundProvider/lib/helpers.tspackages/core-typings/src/ICustomSound.tspackages/models/src/models/CustomSounds.tspackages/ui-contexts/src/CustomSoundContext.tsapps/meteor/app/custom-sounds/server/methods/uploadCustomSound.tsapps/meteor/client/lib/userStatuses.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/livechat/server/business-hour/LivechatBusinessHours.tsapps/meteor/tests/data/livechat/department.tspackages/models/src/models/LivechatBusinessHours.tsapps/meteor/client/lib/userStatuses.ts
📚 Learning: 2025-09-15T21:34:39.812Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 36717
File: packages/ui-voip/src/providers/useCallSounds.ts:6-21
Timestamp: 2025-09-15T21:34:39.812Z
Learning: The voipSounds methods (playDialer, playRinger, playCallEnded) from useCustomSound return proper offCallbackHandler cleanup functions, not void as some type definitions might suggest.
Applied to files:
apps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts
🧬 Code graph analysis (14)
packages/core-services/src/events/Events.ts (1)
packages/core-typings/src/IInquiry.ts (1)
ILivechatInquiryRecord(29-55)
apps/meteor/app/livechat/server/business-hour/LivechatBusinessHours.ts (1)
packages/core-typings/src/ILivechatBusinessHour.ts (1)
ILivechatBusinessHour(33-41)
apps/meteor/app/livechat/server/business-hour/filterBusinessHoursThatMustBeOpened.ts (1)
packages/core-typings/src/ILivechatBusinessHour.ts (1)
ILivechatBusinessHour(33-41)
apps/meteor/app/custom-sounds/server/methods/insertOrUpdateSound.ts (2)
packages/models/src/index.ts (1)
CustomSounds(144-144)packages/core-services/src/index.ts (1)
api(55-55)
apps/meteor/tests/data/livechat/department.ts (1)
packages/core-typings/src/ILivechatDepartment.ts (1)
ILivechatDepartment(3-24)
packages/model-typings/src/models/ILivechatBusinessHoursModel.ts (1)
packages/core-typings/src/ILivechatBusinessHour.ts (1)
ILivechatBusinessHour(33-41)
packages/model-typings/src/models/ICustomSoundsModel.ts (2)
packages/core-typings/src/ICustomSound.ts (1)
ICustomSound(3-8)packages/ui-contexts/src/index.ts (1)
FindOptions(110-110)
packages/models/src/models/LivechatBusinessHours.ts (1)
packages/core-typings/src/ILivechatBusinessHour.ts (1)
ILivechatBusinessHour(33-41)
apps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsx (1)
packages/core-typings/src/ICustomSound.ts (1)
ICustomSound(3-8)
apps/meteor/client/providers/CustomSoundProvider/lib/helpers.ts (1)
packages/core-typings/src/ICustomSound.ts (1)
ICustomSound(3-8)
packages/models/src/models/CustomSounds.ts (1)
packages/core-typings/src/ICustomSound.ts (1)
ICustomSound(3-8)
packages/ui-contexts/src/CustomSoundContext.ts (1)
packages/core-typings/src/ICustomSound.ts (1)
ICustomSound(3-8)
apps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts (2)
packages/models/src/index.ts (1)
CustomSounds(144-144)packages/core-services/src/index.ts (1)
api(55-55)
apps/meteor/client/lib/userStatuses.ts (1)
packages/core-typings/src/ICustomUserStatus.ts (1)
ICustomUserStatus(4-4)
⏰ 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). (10)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (18)
packages/core-services/src/events/Events.ts (1)
26-26: LGTM — consistent replacement ofIInquirywithILivechatInquiryRecord.The import and event signature change on line 74 now aligns with the existing usage on line 242 (
watch.inquiries), andILivechatInquiryRecordproperly extendsIRocketChatRecord(which carries_updatedAt), matching the PR objective.Also applies to: 74-74
packages/core-typings/src/ICustomSound.ts (1)
1-8: LGTM!Clean refactor: extending
IRocketChatRecordcorrectly provides_idand_updatedAtfor this DB document type, and the remaining fields are unchanged. Consumers that don't need_updatedAtproperly useOmit<ICustomSound, '_updatedAt'>elsewhere in the PR.packages/ui-contexts/src/CustomSoundContext.ts (1)
39-39: LGTM!Using
Omit<ICustomSound, '_updatedAt'>for the client-side list is the right approach since the UI context doesn't carry the DB timestamp.packages/core-typings/src/IInstanceStatus.ts (1)
1-23: Correct: intentionally detaching fromIRocketChatRecordsince the instance-status model doesn't support_updatedAt.This is the mirror case of the other changes in this PR — the type now accurately reflects that instance-status documents don't have an
_updatedAtfield.packages/core-typings/src/ILivechatDepartment.ts (1)
1-3: LGTM!Consistent with the PR pattern —
ILivechatDepartmentnow correctly inherits_idand_updatedAtfromIRocketChatRecordas a proper DB document type. The separateLivechatDepartmentDTOremains unchanged, which is correct.packages/core-typings/src/ILivechatBusinessHour.ts (1)
2-2: LGTM!Same consistent pattern —
ILivechatBusinessHournow properly inherits_idand_updatedAtviaIRocketChatRecord.Also applies to: 33-33
packages/model-typings/src/models/ILivechatBusinessHoursModel.ts (1)
26-26: LGTM!Correctly excludes
_updatedAtfrom theinsertOneinput, matching the pattern used inICustomSoundsModel.create— the DB layer auto-sets this field.packages/model-typings/src/models/ICustomSoundsModel.ts (1)
9-10: Signatures match the implementation perfectly.
setNamecorrectly usesfindOneAndUpdatewithreturnDocument: 'after'to return the updated document or null.createcorrectly usesinsertOneand returnsInsertOneResult<WithId<ICustomSound>>. The type omissions for_idand_updatedAtincreate's input accurately reflect DB auto-population behavior.apps/meteor/app/livechat/server/business-hour/LivechatBusinessHours.ts (1)
6-6: LGTM — return type correctly reflects the constructed object shape.The object literal provides
_idbut not_updatedAt, soOmit<ILivechatBusinessHour, '_updatedAt'>is an accurate representation.packages/models/src/models/LivechatBusinessHours.ts (1)
75-80: LGTM — parameter type correctly excludes_updatedAt.The base layer (
BaseRaw.insertOne) handles_idgeneration and_updatedAtstamping, so excluding both from the input type is correct.apps/meteor/app/livechat/server/business-hour/filterBusinessHoursThatMustBeOpened.ts (1)
4-6: LGTM — parameter type widening is correct and consistent with callers.All fields accessed in the body (
active,workHours,_id,type) remain available inOmit<ILivechatBusinessHour, '_updatedAt'>, and this aligns withcreateDefaultBusinessHourRow's updated return type.packages/models/src/models/CustomSounds.ts (2)
35-43: Good refactor —setNamenow returns the updated document.Switching from
updateOnetofindOneAndUpdatewithreturnDocument: 'after'is a clean approach that eliminates the need for a separate fetch when the caller needs the updated document (as ininsertOrUpdateSound.ts).
46-48: LGTM —createparameter type correctly excludes_updatedAt.apps/meteor/app/custom-sounds/server/methods/insertOrUpdateSound.ts (1)
90-96: LGTM — properly handles nullable return and broadcasts full document.The null guard on
updatedSoundcorrectly handles the case where the document wasn't found, and broadcasting the full updated document ensures payload consistency with the actual DB state.apps/meteor/client/providers/CustomSoundProvider/lib/helpers.ts (1)
7-9: No changes needed. The code does not passdefaultSoundselements togetCustomSoundURL. Instead, it passes items fromcustomSoundsList(which are fullICustomSoundobjects from the server methodlistCustomSounds(): ICustomSound[]) to the function, matching its parameter type exactly. ThedefaultSoundsarray is concatenated separately after the mapping operation, so no type mismatch occurs.Likely an incorrect or invalid review comment.
apps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsx (1)
26-26: LGTM — explicit return type aligns queryFn with the updatedICustomSoundshape.The annotation correctly reflects that neither
sdk.call('listCustomSounds')nordefaultSoundsinclude_updatedAt, keeping the type consistent with the broader refactor.apps/meteor/client/lib/userStatuses.ts (1)
36-47: LGTM — parameter type correctly narrowed to exclude_updatedAt.The method only uses
name,_id, andstatusType, soOmit<ICustomUserStatus, '_updatedAt'>accurately describes the required shape. Structural subtyping ensures existing callers (lines 68, 74) remain compatible.packages/core-typings/src/ICustomUserStatus.ts (1)
1-4: Clean consolidation — _updatedAt properly handled by InsertionModel.By extending
IRocketChatRecord,ICustomUserStatusgains a consolidated_updatedAtfield. Although_updatedAtis required in theIRocketChatRecordinterface, theInsertionModel<ICustomUserStatus>type utility already makes_updatedAtoptional during object construction (_updatedAt?: Date), so no call site changes are needed. Existing code that inserts custom user statuses without explicitly providing_updatedAt(e.g.,apps/meteor/app/user-status/server/methods/insertOrUpdateUserStatus.ts) continues to work correctly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![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 22 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/app/custom-sounds/server/methods/uploadCustomSound.ts">
<violation number="1" location="apps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts:32">
P1: The async callback passed to `setTimeout` lacks error handling. If `CustomSounds.findOneById` rejects (e.g., due to a database error), it will result in an UnhandledPromiseRejection, which can crash the Node.js process or leave the application in an unstable state. Wrap the asynchronous operation in a `try/catch` block.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
tassoevan
force-pushed
the
refactor/optional-updated-at-field
branch
from
50d8933 to
3912614
Compare
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/lib/userStatuses.ts (1)
61-70:⚠️ Potential issue | 🔴 CriticalType mismatch in
sync()method:sdk.call('listCustomUserStatus')returnsICustomUserStatus[]butcreateFromCustom()expectsOmit<ICustomUserStatus, '_updatedAt'>.The
listCustomUserStatus()method returns fullICustomUserStatusobjects (which include_updatedAtfromIRocketChatRecord), butcreateFromCustom()at line 36 requiresOmit<ICustomUserStatus, '_updatedAt'>. Passing the full type to a function expecting the omitted type is incompatible.The stream event at line 74 correctly provides
Omit<ICustomUserStatus, '_updatedAt'>and is compatible. Thesync()method should either strip_updatedAtbefore passing tocreateFromCustom(), or the method signature should accept the fullICustomUserStatustype.
🧹 Nitpick comments (1)
apps/meteor/tests/data/livechat/department.ts (1)
200-205: TheOmitis effectively a no-op due to the index signature onILivechatDepartment.Since
ILivechatDepartmentdeclares[k: string]: any,Omit<ILivechatDepartment, '_updatedAt'>still allows_updatedAtto be accessed (asany) through the index signature. The type narrowing has no practical enforcement here—callers can still pass objects with_updatedAt, and line 202 still compiles without issue via the index signature.This isn't a problem introduced by this PR (the index signature is pre-existing), but it's worth noting that the
Omitdoesn't provide the type-safety it suggests. If the index signature onILivechatDepartmentis ever tightened, this will need revisiting.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/meteor/app/custom-sounds/server/methods/insertOrUpdateSound.tsapps/meteor/app/custom-sounds/server/methods/uploadCustomSound.tsapps/meteor/app/livechat/server/business-hour/LivechatBusinessHours.tsapps/meteor/app/livechat/server/business-hour/filterBusinessHoursThatMustBeOpened.tsapps/meteor/client/lib/userStatuses.tsapps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsxapps/meteor/client/providers/CustomSoundProvider/lib/helpers.tsapps/meteor/client/views/admin/workspace/DeploymentCard/components/InstancesModal/InstancesModal.stories.tsxapps/meteor/client/views/admin/workspace/DeploymentCard/components/InstancesModal/InstancesModal.tsxapps/meteor/tests/data/livechat/department.tspackages/core-services/src/events/Events.tspackages/core-typings/src/ICronHistoryItem.tspackages/core-typings/src/ICustomSound.tspackages/core-typings/src/ICustomUserStatus.tspackages/core-typings/src/IInquiry.tspackages/core-typings/src/IInstanceStatus.tspackages/core-typings/src/ILivechatBusinessHour.tspackages/core-typings/src/ILivechatDepartment.tspackages/model-typings/src/models/ICustomSoundsModel.tspackages/model-typings/src/models/ILivechatBusinessHoursModel.tspackages/models/src/models/CustomSounds.tspackages/models/src/models/LivechatBusinessHours.tspackages/ui-contexts/src/CustomSoundContext.ts
💤 Files with no reviewable changes (3)
- apps/meteor/client/views/admin/workspace/DeploymentCard/components/InstancesModal/InstancesModal.stories.tsx
- packages/core-typings/src/IInquiry.ts
- apps/meteor/client/views/admin/workspace/DeploymentCard/components/InstancesModal/InstancesModal.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts
- packages/models/src/models/LivechatBusinessHours.ts
- apps/meteor/app/livechat/server/business-hour/filterBusinessHoursThatMustBeOpened.ts
- packages/model-typings/src/models/ICustomSoundsModel.ts
- packages/core-typings/src/ILivechatBusinessHour.ts
- packages/models/src/models/CustomSounds.ts
- apps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsx
- packages/core-services/src/events/Events.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/app/livechat/server/business-hour/LivechatBusinessHours.tspackages/core-typings/src/ILivechatDepartment.tspackages/ui-contexts/src/CustomSoundContext.tspackages/core-typings/src/ICustomSound.tspackages/core-typings/src/IInstanceStatus.tspackages/core-typings/src/ICronHistoryItem.tsapps/meteor/client/lib/userStatuses.tsapps/meteor/tests/data/livechat/department.tspackages/model-typings/src/models/ILivechatBusinessHoursModel.tspackages/core-typings/src/ICustomUserStatus.tsapps/meteor/app/custom-sounds/server/methods/insertOrUpdateSound.tsapps/meteor/client/providers/CustomSoundProvider/lib/helpers.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/livechat/server/business-hour/LivechatBusinessHours.tspackages/core-typings/src/ICronHistoryItem.tsapps/meteor/client/lib/userStatuses.tsapps/meteor/tests/data/livechat/department.ts
🧬 Code graph analysis (6)
apps/meteor/app/livechat/server/business-hour/LivechatBusinessHours.ts (1)
packages/core-typings/src/ILivechatBusinessHour.ts (1)
ILivechatBusinessHour(33-41)
packages/ui-contexts/src/CustomSoundContext.ts (1)
packages/core-typings/src/ICustomSound.ts (1)
ICustomSound(3-8)
apps/meteor/client/lib/userStatuses.ts (1)
packages/core-typings/src/ICustomUserStatus.ts (1)
ICustomUserStatus(4-4)
apps/meteor/tests/data/livechat/department.ts (1)
packages/core-typings/src/ILivechatDepartment.ts (1)
ILivechatDepartment(3-24)
packages/model-typings/src/models/ILivechatBusinessHoursModel.ts (1)
packages/core-typings/src/ILivechatBusinessHour.ts (1)
ILivechatBusinessHour(33-41)
apps/meteor/client/providers/CustomSoundProvider/lib/helpers.ts (1)
packages/core-typings/src/ICustomSound.ts (1)
ICustomSound(3-8)
⏰ 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 (11)
apps/meteor/app/livechat/server/business-hour/LivechatBusinessHours.ts (1)
6-6: LGTM — return type correctly narrowed.The function never sets
_updatedAt(it's a seed row for DB insertion), soOmit<ILivechatBusinessHour, '_updatedAt'>accurately reflects the runtime shape. Clean alignment with the broaderIRocketChatRecordtype tightening.packages/core-typings/src/ICustomUserStatus.ts (1)
1-4: LGTM — clean centralization of_updatedAtviaIRocketChatRecord.This correctly removes the locally declared optional
_updatedAtin favor of inheriting the required one fromIRocketChatRecord, aligning with the PR's goal of normalizing record types.apps/meteor/client/lib/userStatuses.ts (1)
36-47: Sound type narrowing — method doesn't use_updatedAt.The
Omit<ICustomUserStatus, '_updatedAt'>correctly reflects that this method never reads_updatedAt, and it keeps callers compatible now thatICustomUserStatusinherits a required_updatedAtfromIRocketChatRecord. Objects that do carry_updatedAtremain assignable, so existing call sites at lines 68 and 74 are unaffected.packages/core-typings/src/IInstanceStatus.ts (1)
1-2: Consistent removal of IRocketChatRecord for a type without_updatedAt.Since
_updatedAtis now required onIRocketChatRecord, disconnectingIInstanceStatusand declaring_idexplicitly is the correct approach for documents that don't carry an_updatedAtfield.packages/core-typings/src/ICronHistoryItem.ts (1)
1-3: LGTM!Clean migration to
IRocketChatRecord—_idand_updatedAtare now inherited from the base record type, consistent with the PR's normalization goal.packages/model-typings/src/models/ILivechatBusinessHoursModel.ts (1)
26-26: LGTM!Omitting
_updatedAtfrom theinsertOneparameter is correct — it's auto-managed by the persistence layer, just like_id.packages/core-typings/src/ICustomSound.ts (1)
1-7: LGTM!Promoting
_updatedAtfrom optional to required (viaIRocketChatRecord) correctly reflects the database document shape. Downstream code that constructs objects without_updatedAt(e.g., default sounds) appropriately usesOmit<ICustomSound, '_updatedAt'>.apps/meteor/app/custom-sounds/server/methods/insertOrUpdateSound.ts (1)
90-95: Good improvement: broadcast now sends the actual updated document.Broadcasting the full
updatedSoundreturned bysetNameinstead of a hand-assembled partial object ensures field consistency with theICustomSoundtype. The guard on line 91 is also an improvement — previously the broadcast would fire even if the update was a no-op.One minor note: if
setNamereturns falsy (e.g., document not found), the method still returnssoundData._idon line 98 as if it succeeded. Consider whether this silent no-op should surface an error to the caller.apps/meteor/client/providers/CustomSoundProvider/lib/helpers.ts (1)
28-28: LGTM!
Omit<ICustomSound, '_updatedAt'>correctly models static default sounds that aren't database documents and don't carry_updatedAt.packages/ui-contexts/src/CustomSoundContext.ts (1)
39-39: LGTM!The
listtype is correctly widened toOmit<ICustomSound, '_updatedAt'>[]since it merges default sounds (no_updatedAt) with DB-fetched sounds — the lowest common denominator type is appropriate here.packages/core-typings/src/ILivechatDepartment.ts (1)
1-3: Clean adoption ofIRocketChatRecordas the base type.This correctly centralizes
_idand_updatedAtinto the shared base record, aligning with the PR's goal of making_updatedAta required field on document types. The DTO type below appropriately remains separate since it represents input data, not a stored document.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
The related model is prevented from setting the `_updatedAt` field.
…dated sound object
tassoevan
force-pushed
the
refactor/optional-updated-at-field
branch
from
3912614 to
052278b
Compare
…RocketChatRecord`
…ken` to extend `IRocketChatRecord` and update `create` method to return void
tassoevan
force-pushed
the
refactor/optional-updated-at-field
branch
from
052278b to
e8a28d7
Compare
1 participant
Proposed changes (including videos or screenshots)
Some record types (i.e. types that represent database documents) are not properly declaring the presence of the
_updatedAtfield. This PR normalizes it and refines API methods to reflect it.Issue(s)
ARCH-1931
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
User Interface
Refactor