chore: refactor apps converters to TypeScript with Zod codecs (1/2)#41400
Merged
Conversation
|
|
|
Contributor
WalkthroughAdds bidirectional enum codecs, strengthens TypeScript typing across Apps converters, replaces upload and visitor implementations with TypeScript versions, and adds enum parity tests, converter golden snapshots, and a Zod migration proposal. ChangesApps converter migration
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Contributor
|
Looks like this PR is ready to merge! 🎉 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41400 +/- ##
===========================================
- Coverage 68.47% 68.44% -0.03%
===========================================
Files 4092 4102 +10
Lines 158285 158627 +342
Branches 28622 28829 +207
===========================================
+ Hits 108379 108566 +187
- Misses 44874 44969 +95
- Partials 5032 5092 +60
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d-gubert
changed the title
d-gubert
marked this pull request as ready for review
July 15, 2026 19:14
![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: 4
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/apps/server/converters/messages.ts (2)
207-213: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle a missing editor instead of asserting it exists.
Users.findOneByIdmay returnundefined; the non-null assertions then crash message conversion. Fall back tomessage.editor, or reject with a deliberate validation error.Possible fix
const editor = await Users.findOneById(message.editor.id); + const editorId = editor?._id ?? message.editor.id; + const editorUsername = editor?.username ?? message.editor.username; + + if (!editorUsername) { + throw new Error('Invalid editor provided on the message.'); + } + editedBy = { - _id: editor!._id, - username: editor!.username, + _id: editorId, + username: editorUsername, };🤖 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/apps/server/converters/messages.ts` around lines 207 - 213, Update the editor lookup in the message conversion flow to handle Users.findOneById returning undefined instead of relying on non-null assertions. In the message.editor branch, use the found user when present and fall back to message.editor, or explicitly reject with a deliberate validation error; preserve the existing editedBy shape.
143-160: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
message.ufor the legacy sender fallback.Line 153 deletes
message.u, so Line 160 always callsconvertToApp(undefined)when the primary lookup fails.Proposed fix
sender: async (message: MessageData) => { - if (!message.u?._id) { + const sender = message.u; + if (!sender?._id) { return undefined; } const user = await (isMessageFromVisitor(msgObj) - ? cache.get('user.convertToApp')(message.u) - : cache.get('user.convertById')(message.u._id)); + ? cache.get('user.convertToApp')(sender) + : cache.get('user.convertById')(sender._id)); delete message.u; - return user || cache.get('user.convertToApp')(message.u); + return user || cache.get('user.convertToApp')(sender); },🤖 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/apps/server/converters/messages.ts` around lines 143 - 160, Update the sender resolver in the async sender function to preserve the original message.u value before deleting it, then use that saved value for the legacy fallback when the primary user lookup returns no user. Keep the existing primary lookup and cleanup behavior unchanged, but ensure convertToApp receives the original sender data rather than undefined.apps/meteor/app/apps/server/converters/rooms.ts (1)
154-164: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDefault
visitorChannelInfobefore destructuringvisitorChannelInfois optional and can beundefinedhere, so destructuring it directly can throw. Default it to{}or guard before readinglastMessageTsandphone.🤖 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/apps/server/converters/rooms.ts` around lines 154 - 164, Update __getVisitor to safely handle an undefined visitorChannelInfo before destructuring lastMessageTs and phone, using an empty-object default or an equivalent guard while preserving the existing visitor validation flow.
🧹 Nitpick comments (1)
apps/meteor/app/apps/server/converters/visitors.ts (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove resolved TODO comment.
The
convertVisitorfunction is already correctly handling async behavior by returning the Promise fromtransformMappedData, whileconvertAppVisitoroperates synchronously. Additionally, as per coding guidelines, avoid code comments in the implementation.♻️ Proposed fix
-// TODO: check if functions from this converter can be async export class AppVisitorsConverter implements IAppVisitorsConverter {🤖 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/apps/server/converters/visitors.ts` at line 7, Remove the resolved TODO comment above convertVisitor; retain the existing async behavior through transformMappedData and the synchronous convertAppVisitor implementation unchanged.Source: Coding guidelines
🤖 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/app/apps/server/converters/messages.ts`:
- Around line 12-15: Remove the explanatory migration-rationale comment above
MessageData in apps/meteor/app/apps/server/converters/messages.ts (lines 12-15)
and the corresponding comment above RoomData in
apps/meteor/app/apps/server/converters/rooms.ts (lines 9-11); leave the
converter implementations unchanged.
In `@apps/meteor/app/apps/server/converters/uploads.ts`:
- Around line 89-91: Update the destructuring in the upload conversion flow to
safely handle missing optional references: use optional chaining when extracting
rid from upload.room, consistent with the existing userId and visitorToken
extraction, while preserving the resulting undefined value when the room is
absent.
- Around line 46-53: Update the room converter callback around the room
conversion logic to check whether upload.rid exists before calling convertById.
Return the appropriate empty result when rid is absent, while preserving the
current conversion and rid cleanup behavior when it is present; follow the
existing userId and visitorToken guards as the pattern.
In `@apps/meteor/app/apps/server/converters/users.ts`:
- Line 89: Update the user converter’s utcOffset mapping in
apps/meteor/app/apps/server/converters/users.ts:89 to read user.utcOffset first
and retain user.utfOffset only as a compatibility fallback. Update the primary
round-trip fixture in
apps/meteor/tests/unit/app/apps/server/converters.golden.spec.ts:133-166 to use
utcOffset, and add a focused fallback test if needed to preserve legacy
utfOffset behavior.
---
Outside diff comments:
In `@apps/meteor/app/apps/server/converters/messages.ts`:
- Around line 207-213: Update the editor lookup in the message conversion flow
to handle Users.findOneById returning undefined instead of relying on non-null
assertions. In the message.editor branch, use the found user when present and
fall back to message.editor, or explicitly reject with a deliberate validation
error; preserve the existing editedBy shape.
- Around line 143-160: Update the sender resolver in the async sender function
to preserve the original message.u value before deleting it, then use that saved
value for the legacy fallback when the primary user lookup returns no user. Keep
the existing primary lookup and cleanup behavior unchanged, but ensure
convertToApp receives the original sender data rather than undefined.
In `@apps/meteor/app/apps/server/converters/rooms.ts`:
- Around line 154-164: Update __getVisitor to safely handle an undefined
visitorChannelInfo before destructuring lastMessageTs and phone, using an
empty-object default or an equivalent guard while preserving the existing
visitor validation flow.
---
Nitpick comments:
In `@apps/meteor/app/apps/server/converters/visitors.ts`:
- Line 7: Remove the resolved TODO comment above convertVisitor; retain the
existing async behavior through transformMappedData and the synchronous
convertAppVisitor implementation unchanged.
🪄 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: a4ebe66e-2f67-45c9-a422-484f03b0b8fd
📒 Files selected for processing (14)
apps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/codecs/index.tsapps/meteor/app/apps/server/converters/departments.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/rooms.tsapps/meteor/app/apps/server/converters/settings.tsapps/meteor/app/apps/server/converters/uploads.jsapps/meteor/app/apps/server/converters/uploads.tsapps/meteor/app/apps/server/converters/users.tsapps/meteor/app/apps/server/converters/visitors.jsapps/meteor/app/apps/server/converters/visitors.tsapps/meteor/tests/unit/app/apps/server/codecs/enums.spec.tsapps/meteor/tests/unit/app/apps/server/converters.golden.spec.tsdocs/proposals/apps-converters-zod/README.md
💤 Files with no reviewable changes (2)
- apps/meteor/app/apps/server/converters/uploads.js
- apps/meteor/app/apps/server/converters/visitors.js
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (2/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (5/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (3/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (2/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (4/4)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/unit/app/apps/server/codecs/enums.spec.tsapps/meteor/app/apps/server/converters/codecs/index.tsapps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/uploads.tsapps/meteor/tests/unit/app/apps/server/converters.golden.spec.tsapps/meteor/app/apps/server/converters/visitors.tsapps/meteor/app/apps/server/converters/users.tsapps/meteor/app/apps/server/converters/settings.tsapps/meteor/app/apps/server/converters/rooms.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/departments.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/tests/unit/app/apps/server/codecs/enums.spec.tsapps/meteor/tests/unit/app/apps/server/converters.golden.spec.ts
🧠 Learnings (5)
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/tests/unit/app/apps/server/codecs/enums.spec.tsapps/meteor/tests/unit/app/apps/server/converters.golden.spec.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 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/unit/app/apps/server/codecs/enums.spec.tsapps/meteor/app/apps/server/converters/codecs/index.tsapps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/uploads.tsapps/meteor/tests/unit/app/apps/server/converters.golden.spec.tsapps/meteor/app/apps/server/converters/visitors.tsapps/meteor/app/apps/server/converters/users.tsapps/meteor/app/apps/server/converters/settings.tsapps/meteor/app/apps/server/converters/rooms.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/departments.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/unit/app/apps/server/codecs/enums.spec.tsapps/meteor/app/apps/server/converters/codecs/index.tsapps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/uploads.tsapps/meteor/tests/unit/app/apps/server/converters.golden.spec.tsapps/meteor/app/apps/server/converters/visitors.tsapps/meteor/app/apps/server/converters/users.tsapps/meteor/app/apps/server/converters/settings.tsapps/meteor/app/apps/server/converters/rooms.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/departments.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/tests/unit/app/apps/server/codecs/enums.spec.tsapps/meteor/tests/unit/app/apps/server/converters.golden.spec.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/unit/app/apps/server/codecs/enums.spec.tsapps/meteor/app/apps/server/converters/codecs/index.tsapps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/uploads.tsapps/meteor/tests/unit/app/apps/server/converters.golden.spec.tsapps/meteor/app/apps/server/converters/visitors.tsapps/meteor/app/apps/server/converters/users.tsapps/meteor/app/apps/server/converters/settings.tsapps/meteor/app/apps/server/converters/rooms.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/departments.ts
🔇 Additional comments (10)
apps/meteor/app/apps/server/converters/codecs/index.ts (1)
1-2: LGTM!apps/meteor/tests/unit/app/apps/server/codecs/enums.spec.ts (1)
93-103: LGTM!apps/meteor/app/apps/server/converters/departments.ts (1)
1-27: LGTM!docs/proposals/apps-converters-zod/README.md (1)
1-5: LGTM!apps/meteor/app/apps/server/converters/codecs/enums.ts (1)
26-26: 📐 Maintainability & Code QualityCheck the Zod module augmentation for
z.codec.z.codec,z.decode, andz.encodeare not standard Zod APIs, so this file should only rely on them if the project adds the correspondingzodmodule augmentation.apps/meteor/app/apps/server/converters/settings.ts (1)
1-17: LGTM!Also applies to: 31-34, 51-51
apps/meteor/app/apps/server/converters/users.ts (1)
1-73: LGTM!Also applies to: 90-121, 138-138
apps/meteor/app/apps/server/converters/messages.ts (1)
1-3: LGTM!Also applies to: 16-41, 67-128, 162-173, 231-231, 263-266, 306-306, 333-333, 342-342, 351-364
apps/meteor/app/apps/server/converters/rooms.ts (1)
1-8: LGTM!Also applies to: 12-40, 75-75, 90-137, 177-177, 193-193, 219-219, 228-244, 301-312, 338-348, 358-363, 374-382, 383-391, 398-398, 412-412, 423-437, 448-458, 459-459, 470-470, 486-491
apps/meteor/tests/unit/app/apps/server/converters.golden.spec.ts (1)
1-132: LGTM!Also applies to: 167-867
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 14 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
Detailed plan behind PR #41205: problem, codec design decisions, the phased rollout (Phase 0 done), testing strategy via golden snapshots, and risks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185hwJYnx3nzP4pcrYM2XXj move document
Groundwork for migrating the apps-engine converters to Zod codecs. No runtime behaviour changes — this only converts files to TypeScript and adds scaffolding + a behavioural safety net for the later phases. - Convert the 7 remaining .js converters (settings, users, visitors, departments, uploads, rooms, messages) to .ts, preserving behaviour verbatim (including the legacy `utfOffset` read and throw-on-null paths). Transform maps in rooms/messages keep loose typing for now; they are tightened when each is codec-ified. - Add converters/codecs/ with the first shared primitives: bidirectional enum codecs (UserType, UserStatusConnection, RoomType, SettingType) that reproduce the current _convert* switch logic exactly. - Add unit tests asserting the enum codecs match the legacy helpers, and golden-snapshot tests locking the current RC <-> Apps-Engine field mapping (incl. the _unmappedProperties_ bucket) for settings, users, visitors, departments, roles, videoConferences, contacts and uploads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185hwJYnx3nzP4pcrYM2XXj test(apps): add decode golden snapshots for rooms, messages and threads The golden equivalence oracle previously covered only the simpler converters; the async, cross-converter, memoized ones (rooms, messages, threads) — where mapping drift is hardest to reason about — had no golden coverage, and threads had no test at all. Add decode-direction golden snapshots (via the stub orchestrator, so they run without a database) locking the RC -> Apps-Engine mapping for: - AppRoomsConverter.convertRoom (channel + livechat: renames, boolean/type function fields, cross-converter lookups, secure fields, bucket) and convertRoomRaw; - AppMessagesConverter.convertMessage (room/editor/sender lookups, attachment sub-map, bucket) and convertMessageRaw; - AppThreadsConverter.convertMessage (injected room, passed-in user lookups, attachment sub-map, bucket) — previously untested. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185hwJYnx3nzP4pcrYM2XXj
…tatus case
Address PR 41400 review feedback:
- uploads: guard `upload.rid` in the `room` mapper before calling
`convertById`, matching the `user`/`visitor` handlers. `IUpload['rid']`
is optional, so the previous `as string` cast masked a possible
`undefined` reaching the rooms lookup.
- users: guard `convertByUsername` against an empty/undefined username
instead of casting with `as string`, making the contract explicit and
avoiding a `{ username: undefined }` query.
- tests: add the empty-string input to the UserStatusConnectionCodec
decode test so the falsy `!status -> OFFLINE` branch is covered.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0158L8oVaauRgodsw6bVvZii
Address additional PR 41400 review feedback (crash-prevention only):
- uploads: default `upload.room` to `{}` when destructuring `rid` in
convertToRocketChat, matching the sibling `upload.user`/`upload.visitor`
guards. `IUpload['room']` can be absent at runtime (uploads without a
room), which previously threw a TypeError.
- rooms: default `visitorChannelInfo` to `{}` in `__getVisitor` before
destructuring `lastMessageTs`/`phone`, consistent with the method's
existing `roomVisitor`/`visitor` guards.
These preserve the intended result on all non-crashing paths (verified by
the golden snapshot tests) and only turn a latent TypeError into the
already-intended empty result.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0158L8oVaauRgodsw6bVvZii
These were previously carried forward as "behaviour to preserve"; typing the converters made them visible, so fix them with focused coverage: - users: convertToRocketChat read the misspelled `utfOffset`, so a round-tripped user always lost `utcOffset`. Read `utcOffset` first and keep `utfOffset` only as a compatibility fallback. - messages: the visitor-sender fallback re-read `message.u` after it had been deleted, so it always received `undefined`. Capture the sender before deletion. - messages: convertAppMessage dereferenced Users.findOneById through a non-null assertion, throwing when the editor no longer exists. Fall back to the editor data on the app payload, mirroring the sender handling. Update the golden user round-trip fixture to `utcOffset`, add focused tests for the `utfOffset` fallback and the sender fallback, and document the fixes in the migration proposal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0158L8oVaauRgodsw6bVvZii
d-gubert
force-pushed
the
chore/apps-converters-zod-1
branch
from
July 16, 2026 15:54
64fd256 to
407662b
Compare
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
ggazzo
approved these changes
Jul 16, 2026
d-gubert
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
sampaiodiego
approved these changes
Jul 16, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 16, 2026
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
5 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)
Proposed changes
This PR adds the proposal document and the foundation to support migration of the apps converters from JavaScript to TypeScript and introduces Zod-based codec infrastructure for enum mappings. The changes are behavior-preserving, with the exception of a small set of latent bugs that typing the converters made visible — those are fixed deliberately and each is pinned by a test (see Incidental bug fixes).
Key changes:
TypeScript migration: Converted the following converters from
.jsto.ts:AppUsersConverterAppMessagesConverterAppRoomsConverterAppSettingsConverterAppDepartmentsConverterAppVisitorsConverterAppUploadsConverterNew Zod codec infrastructure (
app/apps/server/converters/codecs/enums.ts):UserTypeCodec: Maps Rocket.Chat user type strings to Apps-EngineUserTypeenumUserStatusConnectionCodec: Maps status connection strings toUserStatusConnectionenumRoomTypeCodec: Maps room type characters toRoomTypeenumSettingTypeCodec: Maps setting type strings toSettingTypeenumComprehensive golden snapshot tests (
tests/unit/app/apps/server/converters.golden.spec.ts):_unmappedProperties_bucketingEnum codec tests (
tests/unit/app/apps/server/codecs/enums.spec.ts):decodeandencodeoperationsIncidental bug fixes
Typing the converters surfaced three pre-existing latent bugs. Rather than carry them forward as "behavior to preserve", they are fixed here — these are the only intentional behavior changes in the PR, and each is covered by a golden or focused test:
users—utfOffsettypo (from-app).convertToRocketChatread the misspelledappUser.utfOffset, so a round-tripped user always dropped itsutcOffset. It now readsutcOffsetfirst and keepsutfOffsetonly as a compatibility fallback for any app still emitting the old key. The golden round-trip fixture now usesutcOffset; a focused test pins the legacy fallback.messages— dead visitor-sender fallback (to-app). Thesenderresolver deletedmessage.ubefore the "old system message without token" fallback re-read it, so the fallback always receivedundefined. It now captures the sender before deletion; a focused test drives a missing primary lookup and asserts the fallback resolves the original sender.messages— null editor deref (from-app).convertAppMessagedereferencedUsers.findOneById(editor.id)through a non-null assertion, throwing when the editor no longer exists. It now falls back to the editor data carried on the app payload, mirroring the adjacent sender handling.Alongside these, a few defensive guards were added where a destructuring or lookup could hit an optional value the types claimed was always present (
uploadsrid/room,usersconvertByUsername,roomsvisitorChannelInfo). These only turn a latentTypeErrorinto the already-intended empty result and match the guards their sibling fields already had, so they change no covered output.Migration approach:
_unmappedProperties_bucketing for forward compatibility_convert*ToApp/_convert*ToEnumhelpersIssue(s)
https://rocketchat.atlassian.net/browse/ARCH-2265
#41205
Steps to test or reproduce
Further comments
<img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg">Summary by CodeRabbit
New Features
Bug Fixes
utcOffsetround-trip typo, a dead visitor-sender fallback, and a null-editor dereference in the message/user converters.Tests
Documentation
Summary by CodeRabbit