refactor(core-typings): Cloud-related schemas #38219
Conversation
|
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 |
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces Serialized wrappers with Zod-based Cloud.*Schema validation across cloud sync/license flows, returns schema-validated payloads, adds Authorization and version handling to cloud requests, simplifies error classes by overriding their name, and updates handler signatures to accept the new typed payloads. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant CloudAPI as CloudAPI
participant Validator as SchemaValidator
participant Handlers as CommsHandlers
participant DB as Database
Client->>CloudAPI: GET/POST (Authorization: Bearer <token>, ?version=LICENSE_VERSION, body)
CloudAPI-->>Client: HTTP response (payload or error)
Client->>Validator: Cloud.*Schema.safeParse(payload)
alt validation failed
Validator-->>Client: prettified Zod error (cause)
Client->>Client: throw CloudWorkspaceConnectionError / CloudWorkspaceLicenseError
else validation ok
Validator-->>Client: result.data (typed payload)
Client->>Handlers: pass typed payload (NPS, banners, announcements, license)
Handlers->>DB: create/update/disable records
DB-->>Handlers: ack
Handlers-->>Client: completion
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38219 +/- ##
===========================================
+ Coverage 70.75% 70.77% +0.01%
===========================================
Files 3142 3142
Lines 108926 108926
Branches 19581 19653 +72
===========================================
+ Hits 77072 77093 +21
+ Misses 29857 29831 -26
- Partials 1997 2002 +5
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", "01/21 21:10 (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: Wed, 21 Jan 2026 21:10:15 GMT |
tassoevan
force-pushed
the
refactor/cloud-schemas
branch
3 times, most recently
from
df125d4 to
d95a2ed
Compare
![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 19 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="packages/core-typings/src/cloud/WorkspaceSyncPayload.ts">
<violation number="1" location="packages/core-typings/src/cloud/WorkspaceSyncPayload.ts:74">
P2: `announcements` was optional in the previous payload shape, but the new schema makes it required. With Zod parsing this will reject valid responses that omit `announcements`. Make the announcements object optional to preserve the original contract.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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 (3)
apps/meteor/app/cloud/server/functions/getWorkspaceLicense.ts (1)
25-32: Error handling swallows the original error.The
catchblock on line 29 catches any error, including theCloudWorkspaceConnectionErrorthrown on line 28. Whenresponse.json()succeeds but the subsequentthrowis caught by the sametry-catch, the original error message is lost and replaced withresponse.statusText.Proposed fix
if (!response.ok) { - try { - const { error } = await response.json(); - throw new CloudWorkspaceConnectionError(`Failed to connect to Rocket.Chat Cloud: ${error}`); - } catch (error) { - throw new CloudWorkspaceConnectionError(`Failed to connect to Rocket.Chat Cloud: ${response.statusText}`); - } + const { error } = await response.json().catch(() => ({ error: response.statusText })); + throw new CloudWorkspaceConnectionError(`Failed to connect to Rocket.Chat Cloud: ${error}`); }apps/meteor/app/cloud/server/functions/syncWorkspace/legacySyncWorkspace.ts (1)
35-42: Same error handling issue as ingetWorkspaceLicense.ts.The
catchblock catches all errors including theCloudWorkspaceConnectionErrorthrown on line 38, replacing it with a genericstatusTexterror.Proposed fix
if (!response.ok) { - try { - const { error } = await response.json(); - throw new CloudWorkspaceConnectionError(`Failed to connect to Rocket.Chat Cloud: ${error}`); - } catch (error) { - throw new CloudWorkspaceConnectionError(`Failed to connect to Rocket.Chat Cloud: ${response.statusText}`); - } + const { error } = await response.json().catch(() => ({ error: response.statusText })); + throw new CloudWorkspaceConnectionError(`Failed to connect to Rocket.Chat Cloud: ${error}`); }apps/meteor/app/cloud/server/functions/syncWorkspace/fetchWorkspaceSyncPayload.ts (1)
24-27: Unhandled JSON parse error on non-OK response.If the error response body is not valid JSON,
response.json()will throw, and the error message context will be lost. Consider wrapping this in a.catch()similar to the proposed fix for other files.Proposed fix
if (!response.ok) { - const { error } = await response.json(); + const { error } = await response.json().catch(() => ({ error: response.statusText })); throw new CloudWorkspaceConnectionError(`Failed to connect to Rocket.Chat Cloud: ${error}`); }
🧹 Nitpick comments (5)
packages/core-typings/src/cloud/index.ts (1)
1-6: Consider re-exportingNpsSurveyAnnouncementSchemafor API consistency.Since this index now exposes schema exports for other cloud payloads, adding
NpsSurveyAnnouncementSchemahere would give consumers a single import path for both the schema and inferred type.packages/core-typings/src/IBanner.ts (1)
26-26:z.custom<UiKit.BannerView>()provides no runtime validation.Using
z.custom<T>()without a validation function only provides type inference—any value will pass validation at runtime. If theviewfield requires structural validation, consider adding a refinement or a proper schema.packages/core-typings/src/cloud/WorkspaceSyncPayload.ts (1)
53-59: Consider typingcloudSyncAnnouncementmore precisely.Using
z.unknown()forcloudSyncAnnouncement(line 58) provides no type safety or validation. If this field has a known structure, consider defining a schema for it. If it's intentionally untyped, this is acceptable but worth documenting.apps/meteor/app/cloud/server/functions/syncWorkspace/fetchWorkspaceSyncPayload.ts (1)
16-22: Consider adding a timeout to the fetch call.Other similar functions in this codebase (e.g.,
fetchWorkspaceClientPayloadinlegacySyncWorkspace.ts) specifytimeout: 5000. Missing a timeout here could lead to indefinitely hanging requests.Proposed fix
const response = await fetch(`${workspaceRegistrationClientUri}/sync`, { method: 'POST', headers: { Authorization: `Bearer ${token}`, }, body: data, + timeout: 5000, });apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts (1)
32-57: ResolvecreatedBytype mismatch instead of casting toany.
The cast hides shape drift and bypasses type safety. Consider mappingCloud.Announcement['createdBy']intoIBanner['createdBy']or aligning the schema types.If you want, I can propose a concrete mapping or schema tweak to remove the cast.
tassoevan
force-pushed
the
refactor/cloud-schemas
branch
from
d95a2ed to
6c94bfe
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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/cloud/server/functions/syncWorkspace/handleCommsSync.ts (1)
43-56: Address thecreatedBytype incompatibility instead of casting toany.The
as anycast on line 53 suppresses a legitimate type mismatch betweenAnnouncement.createdBy(which is'cloud' | 'system'perAnnouncementSchema) andIBanner.createdBy(which expects an object with_idand optionalusername). This will cause runtime issues whenBanner.createexpects an object but receives a string.Run the following to verify the type definitions:
#!/bin/bash # Check AnnouncementSchema createdBy type rg -n "createdBy" packages/core-typings/src/cloud/Announcement.ts # Check IBannerSchema createdBy type rg -n "createdBy" packages/core-typings/src/IBanner.ts # Check Banner.create signature ast-grep --pattern $'Banner.create($$$)'
🤖 Fix all issues with AI agents
In
`@apps/meteor/app/cloud/server/functions/syncWorkspace/fetchWorkspaceSyncPayload.ts`:
- Around line 34-36: The thrown CloudWorkspaceConnectionError has the wrong
schema name in its message; update the message to reference
Cloud.WorkspaceSyncResponseSchema instead of WorkspaceSyncPayloadSchema where
the error is constructed (the throw using CloudWorkspaceConnectionError and
z.prettifyError(assertWorkspaceSyncPayload.error)). Ensure the new message reads
something like "Cloud.WorkspaceSyncResponseSchema failed type validation" while
keeping the z.prettifyError(assertWorkspaceSyncPayload.error) as the cause.
In `@packages/core-typings/src/cloud/Announcement.ts`:
- Around line 5-6: AnnouncementSchema currently overrides
IBannerSchema.createdBy (an object with _id and optional username) with a string
enum, breaking compatibility; update AnnouncementSchema to accept both shapes by
changing the createdBy field to a union of the original object shape and the
enum (or stop extending IBannerSchema if they truly diverge). Locate
AnnouncementSchema and IBannerSchema symbols and modify AnnouncementSchema's
createdBy to a z.union(...) of the object form (matching
IBannerSchema.createdBy) and z.enum(['cloud','system']), or remove the
IBannerSchema.extend usage and define a standalone schema if you intend full
divergence.
🧹 Nitpick comments (4)
apps/meteor/app/cloud/server/functions/syncWorkspace/announcementSync.ts (1)
41-49: Clarify the schema name in the validation error.
The message referencesWorkspaceCommPayloadSchema, but the actual validator isWorkspaceCommsResponsePayloadSchema. Updating the message helps debugging.Suggested tweak
- throw new CloudWorkspaceConnectionError('WorkspaceCommPayloadSchema failed type validation', { + throw new CloudWorkspaceConnectionError('WorkspaceCommsResponsePayloadSchema failed type validation', { cause: z.prettifyError(result.error), });packages/core-typings/src/cloud/Announcement.ts (1)
7-7:selector.rolesis required whenselectoris present; consider making it optional for consistency with the deprecatedrolesfield.The current schema requires the
rolesarray when aselectorobject is provided. Since the deprecatedrolesfield inIBannerSchema(line 20 of IBanner.ts) uses.optional(), consider applying the same pattern here to allow for flexible selector definitions and align with existing design conventions.Suggested change
- selector: z.object({ roles: z.array(z.string()) }).optional(), + selector: z.object({ roles: z.array(z.string()).optional() }).optional(),packages/core-typings/src/cloud/WorkspaceSyncPayload.ts (1)
53-61: Consider defining a more specific schema forcloudSyncAnnouncement.Using
z.unknown()forcloudSyncAnnouncementbypasses validation entirely. If the structure is known, a more specific schema would provide better type safety and catch malformed payloads early.packages/core-typings/src/IBanner.ts (1)
13-32:z.custom()forviewprovides no runtime validation.Using
z.custom<UiKit.BannerView>()on line 26 provides type inference but no runtime validation. If the incoming payload has malformedviewdata, it will pass validation and potentially cause runtime errors downstream. Consider adding a refinement or basic structural validation if the view structure is known.Otherwise, the schema correctly uses Zod 4's
z.enum()with TypeScript enums (line 16),TimestampSchemafor date fields, and.meta({ deprecated: true })for deprecation marking.
apps/meteor/app/cloud/server/functions/syncWorkspace/fetchWorkspaceSyncPayload.ts
Outdated
Show resolved
Hide resolved
tassoevan
force-pushed
the
refactor/cloud-schemas
branch
from
6c94bfe to
c90f639
Compare
tassoevan
force-pushed
the
refactor/cloud-schemas
branch
from
d59eaaf to
857a502
Compare
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 (2)
apps/meteor/app/cloud/server/functions/getWorkspaceLicense.ts (1)
25-32: Error handling bug: the catch block swallows the specific error message.If
response.json()succeeds and throwsCloudWorkspaceConnectionErrorwith the parsed error, the catch block immediately catches that same error and replaces it with a genericstatusTextmessage, losing the specific error details.🐛 Proposed fix
if (!response.ok) { - try { - const { error } = await response.json(); - throw new CloudWorkspaceConnectionError(`Failed to connect to Rocket.Chat Cloud: ${error}`); - } catch (error) { - throw new CloudWorkspaceConnectionError(`Failed to connect to Rocket.Chat Cloud: ${response.statusText}`); - } + const { error } = await response.json().catch(() => ({ error: undefined })); + throw new CloudWorkspaceConnectionError(`Failed to connect to Rocket.Chat Cloud: ${error ?? response.statusText}`); }apps/meteor/app/cloud/server/functions/syncWorkspace/fetchWorkspaceSyncPayload.ts (1)
24-27: Error handling may throw on invalid JSON response.If
response.json()fails (e.g., non-JSON error response) or the response body doesn't contain anerrorfield, this code will throw an unhandled exception instead of the intendedCloudWorkspaceConnectionError.Suggested defensive handling
if (!response.ok) { - const { error } = await response.json(); - throw new CloudWorkspaceConnectionError(`Failed to connect to Rocket.Chat Cloud: ${error}`); + try { + const { error } = await response.json(); + throw new CloudWorkspaceConnectionError(`Failed to connect to Rocket.Chat Cloud: ${error}`); + } catch (e) { + if (e instanceof CloudWorkspaceConnectionError) { + throw e; + } + throw new CloudWorkspaceConnectionError(`Failed to connect to Rocket.Chat Cloud: ${response.statusText}`); + } }
🤖 Fix all issues with AI agents
In `@packages/core-typings/src/IBanner.ts`:
- Line 16: The platform field currently uses z.enum(BannerPlatform) which is
incorrect for a TypeScript enum; replace that usage with
z.nativeEnum(BannerPlatform) (or an explicit literal array) so the schema
validates the BannerPlatform enum correctly—update the platform property in the
IBanner schema to use z.nativeEnum(BannerPlatform) instead of
z.enum(BannerPlatform) and run type checks to ensure consistency with other
z.enum usages.
🧹 Nitpick comments (3)
packages/core-typings/src/cloud/WorkspaceLicensePayload.ts (1)
5-11: LGTM!Clean Zod schema definition. The
TimestampSchemausage for date fields is consistent with the pattern established in this PR.Optional consideration: If
versionshould always be a non-negative integer, you could usez.number().int().nonnegative()for stricter validation, but this may be intentionally flexible.packages/core-typings/src/cloud/WorkspaceSyncPayload.ts (2)
56-56: Consider typingcloudSyncAnnouncementmore specifically.Using
z.unknown()forcloudSyncAnnouncementloses type safety and validation. If the structure is known, consider defining a proper schema. If the structure truly varies or is externally defined, this is acceptable but worth documenting.
37-49: Consider convertingWorkspaceSyncRequestPayloadto a Zod schema for consistency.Unlike other payload definitions in this file that use Zod schemas,
WorkspaceSyncRequestPayloadremains a plain TypeScript type. For consistency with the refactoring pattern and to enable runtime validation of outgoing requests, consider defining it as a Zod schema.Example schema conversion
export const WorkspaceSyncRequestPayloadSchema = z.object({ uniqueId: z.string(), workspaceId: z.string(), seats: z.number(), MAC: z.number(), address: z.string(), siteName: z.string(), deploymentMethod: z.string(), deploymentPlatform: z.string(), version: z.string(), licenseVersion: z.number(), connectionDisable: z.boolean(), }); export type WorkspaceSyncRequestPayload = z.infer<typeof WorkspaceSyncRequestPayloadSchema>;
Proposed changes (including videos or screenshots)
It moves Zod schemas for cloud-related types to
@rocket.chat/core-typings. Additionally, it uses Zod parsing for payload deserialization.Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.