chore: refactor apps converters to TypeScript with Zod codecs (2/2)#41205
Open
d-gubert wants to merge 12 commits into
Open
chore: refactor apps converters to TypeScript with Zod codecs (2/2)#41205d-gubert wants to merge 12 commits into
d-gubert wants to merge 12 commits into
Conversation
Contributor
|
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 |
|
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds shared Zod codecs and mapped-data helpers, migrates Apps-Engine converters from ChangesApp converter codec migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AppConverter
participant ZodCodec
participant AppOrchestrator
participant RelatedConverter
AppConverter->>ZodCodec: decode or encode entity
ZodCodec->>AppOrchestrator: resolve related identifiers
AppOrchestrator->>RelatedConverter: convert related entity
RelatedConverter-->>ZodCodec: return converted entity
ZodCodec-->>AppConverter: return mapped result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
d-gubert
pushed a commit
that referenced
this pull request
Jul 6, 2026
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41205 +/- ##
===========================================
+ Coverage 68.52% 68.73% +0.21%
===========================================
Files 4128 4149 +21
Lines 160182 159196 -986
Branches 29148 28080 -1068
===========================================
- Hits 109760 109422 -338
+ Misses 45319 44622 -697
- Partials 5103 5152 +49
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d-gubert
pushed a commit
that referenced
this pull request
Jul 9, 2026
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
d-gubert
force-pushed
the
claude/converters-zod-migration-p29du9
branch
from
July 9, 2026 15:34
a263b95 to
82ae480
Compare
d-gubert
changed the title
d-gubert
pushed a commit
that referenced
this pull request
Jul 14, 2026
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
d-gubert
force-pushed
the
claude/converters-zod-migration-p29du9
branch
2 times, most recently
from
July 15, 2026 18:13
5b2db8a to
880e195
Compare
d-gubert
changed the title
d-gubert
force-pushed
the
claude/converters-zod-migration-p29du9
branch
from
July 16, 2026 15:04
880e195 to
f773e1a
Compare
d-gubert
pushed a commit
that referenced
this pull request
Jul 16, 2026
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
d-gubert
force-pushed
the
chore/apps-converters-zod-1
branch
from
July 16, 2026 15:54
64fd256 to
407662b
Compare
d-gubert
force-pushed
the
claude/converters-zod-migration-p29du9
branch
from
July 16, 2026 15:54
f773e1a to
e2ccedd
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![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: 6
🧹 Nitpick comments (2)
apps/meteor/app/apps/server/converters/rooms.ts (1)
53-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the implementation comment.
The partial/full branch is already self-explanatory. As per coding guidelines, “Avoid code comments in the implementation.”
Proposed change
- // The codec's `encode` covers the full (non-partial) conversion; the partial variant cannot be - // expressed through the codec interface, so it calls the shared transform directly.🤖 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 53 - 54, Remove the implementation comment describing the codec encode behavior and partial conversion near the partial/full conversion branch in rooms.ts, leaving the existing logic unchanged.Source: Coding guidelines
apps/meteor/tests/unit/app/apps/server/converters.edge.spec.ts (1)
9-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the explanatory code comments.
The descriptive suite and test names already communicate this intent. As per coding guidelines, “Avoid code comments in the implementation.”
Also applies to: 18-19
🤖 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/unit/app/apps/server/converters.edge.spec.ts` around lines 9 - 14, Remove the explanatory block comments describing lenient enum handling and pre-migration behavior in the affected test file, including the additional commented lines noted in the review; leave the descriptive suite and test names and all test logic 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/codecs/contacts.ts`:
- Line 101: Update the contact encoding path around mappedDecodeAsync to enable
a recursive drop-unmapped mode, ensuring _unmappedProperties_ is omitted at the
root and all nested levels. Extend the helper with an option for this behavior
while preserving its current default for other callers, then pass that option
when encoding ILivechatContact values.
In `@apps/meteor/app/apps/server/converters/codecs/mappedData.ts`:
- Around line 140-142: Update the nested mapping branch in mappedDecodeAsync to
check clone[fromName] before recursively calling mappedDecodeAsync, preserving
absent undefined and null values without property access errors. Add regression
coverage for both missing and null nested objects, including optional fields
such as lastChat, visitor, or details.
In `@apps/meteor/app/apps/server/converters/codecs/rooms.ts`:
- Around line 344-380: Update the room relationship converters to consume source
keys before unmapped properties are merged: in the contact mapping, read and
remove contactId before returning the converted contact; in visitorChannelInfo,
read and remove v only after both visitor mappings have consumed it. Replace
duplicated v extraction/removal with a named helper, and do not add
implementation comments.
In `@apps/meteor/app/apps/server/converters/codecs/uploads.ts`:
- Around line 70-89: Update the upload encoding mapping that constructs
newUpload to include the upload.path value. Preserve the existing field mappings
and ensure path survives encode/decode round trips.
In `@apps/meteor/app/apps/server/converters/users.ts`:
- Line 54: Update UserCodec.encode or the surrounding conversion flow to fall
back from the legacy utfOffset property to utcOffset before encoding, preserving
existing utcOffset behavior. Add a regression test covering users that provide
only utfOffset.
In `@docs/proposals/apps-converters-zod/README.md`:
- Around line 275-278: Revise the “apps-type boundary keeps a single, documented
cast” statement in the README to clarify that each affected codec
boundary—supplied role, department, user, and visitor—retains one localized
cast. Replace the globally implying “exactly one” wording while preserving the
explanation that these casts are deliberate seams for the hand-authored
Apps-Engine interfaces and reference to design decision 7.
---
Nitpick comments:
In `@apps/meteor/app/apps/server/converters/rooms.ts`:
- Around line 53-54: Remove the implementation comment describing the codec
encode behavior and partial conversion near the partial/full conversion branch
in rooms.ts, leaving the existing logic unchanged.
In `@apps/meteor/tests/unit/app/apps/server/converters.edge.spec.ts`:
- Around line 9-14: Remove the explanatory block comments describing lenient
enum handling and pre-migration behavior in the affected test file, including
the additional commented lines noted in the review; leave the descriptive suite
and test names and all test logic 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: c30268fe-1f44-46e5-9414-28e9bd6408c3
📒 Files selected for processing (28)
apps/meteor/app/apps/server/converters/codecs/contacts.tsapps/meteor/app/apps/server/converters/codecs/departments.tsapps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/codecs/index.tsapps/meteor/app/apps/server/converters/codecs/mappedData.tsapps/meteor/app/apps/server/converters/codecs/roles.tsapps/meteor/app/apps/server/converters/codecs/rooms.tsapps/meteor/app/apps/server/converters/codecs/settings.tsapps/meteor/app/apps/server/converters/codecs/uploads.tsapps/meteor/app/apps/server/converters/codecs/users.tsapps/meteor/app/apps/server/converters/codecs/videoConferences.tsapps/meteor/app/apps/server/converters/codecs/visitors.tsapps/meteor/app/apps/server/converters/contacts.tsapps/meteor/app/apps/server/converters/departments.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/roles.tsapps/meteor/app/apps/server/converters/rooms.tsapps/meteor/app/apps/server/converters/settings.tsapps/meteor/app/apps/server/converters/threads.tsapps/meteor/app/apps/server/converters/transformMappedData.tsapps/meteor/app/apps/server/converters/uploads.tsapps/meteor/app/apps/server/converters/users.tsapps/meteor/app/apps/server/converters/videoConferences.tsapps/meteor/app/apps/server/converters/visitors.tsapps/meteor/tests/unit/app/apps/server/codecs/mappedData.spec.tsapps/meteor/tests/unit/app/apps/server/converters.edge.spec.tsapps/meteor/tests/unit/server/lib/import/transformMappedData.spec.tsdocs/proposals/apps-converters-zod/README.md
💤 Files with no reviewable changes (2)
- apps/meteor/app/apps/server/converters/transformMappedData.ts
- apps/meteor/tests/unit/server/lib/import/transformMappedData.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: 📦 Build Packages
🧰 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/app/apps/server/converters/codecs/videoConferences.tsapps/meteor/app/apps/server/converters/codecs/roles.tsapps/meteor/app/apps/server/converters/codecs/contacts.tsapps/meteor/app/apps/server/converters/codecs/index.tsapps/meteor/app/apps/server/converters/codecs/visitors.tsapps/meteor/tests/unit/app/apps/server/converters.edge.spec.tsapps/meteor/tests/unit/app/apps/server/codecs/mappedData.spec.tsapps/meteor/app/apps/server/converters/codecs/users.tsapps/meteor/app/apps/server/converters/codecs/settings.tsapps/meteor/app/apps/server/converters/codecs/uploads.tsapps/meteor/app/apps/server/converters/roles.tsapps/meteor/app/apps/server/converters/visitors.tsapps/meteor/app/apps/server/converters/uploads.tsapps/meteor/app/apps/server/converters/codecs/departments.tsapps/meteor/app/apps/server/converters/videoConferences.tsapps/meteor/app/apps/server/converters/threads.tsapps/meteor/app/apps/server/converters/contacts.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/departments.tsapps/meteor/app/apps/server/converters/codecs/mappedData.tsapps/meteor/app/apps/server/converters/users.tsapps/meteor/app/apps/server/converters/codecs/rooms.tsapps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/settings.tsapps/meteor/app/apps/server/converters/rooms.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/converters.edge.spec.tsapps/meteor/tests/unit/app/apps/server/codecs/mappedData.spec.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/app/apps/server/converters/codecs/videoConferences.tsapps/meteor/app/apps/server/converters/codecs/roles.tsapps/meteor/app/apps/server/converters/codecs/contacts.tsapps/meteor/app/apps/server/converters/codecs/index.tsapps/meteor/app/apps/server/converters/codecs/visitors.tsapps/meteor/tests/unit/app/apps/server/converters.edge.spec.tsapps/meteor/tests/unit/app/apps/server/codecs/mappedData.spec.tsapps/meteor/app/apps/server/converters/codecs/users.tsapps/meteor/app/apps/server/converters/codecs/settings.tsapps/meteor/app/apps/server/converters/codecs/uploads.tsapps/meteor/app/apps/server/converters/roles.tsapps/meteor/app/apps/server/converters/visitors.tsapps/meteor/app/apps/server/converters/uploads.tsapps/meteor/app/apps/server/converters/codecs/departments.tsapps/meteor/app/apps/server/converters/videoConferences.tsapps/meteor/app/apps/server/converters/threads.tsapps/meteor/app/apps/server/converters/contacts.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/departments.tsapps/meteor/app/apps/server/converters/codecs/mappedData.tsapps/meteor/app/apps/server/converters/users.tsapps/meteor/app/apps/server/converters/codecs/rooms.tsapps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/settings.tsapps/meteor/app/apps/server/converters/rooms.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/app/apps/server/converters/codecs/videoConferences.tsapps/meteor/app/apps/server/converters/codecs/roles.tsapps/meteor/app/apps/server/converters/codecs/contacts.tsapps/meteor/app/apps/server/converters/codecs/index.tsapps/meteor/app/apps/server/converters/codecs/visitors.tsapps/meteor/tests/unit/app/apps/server/converters.edge.spec.tsapps/meteor/tests/unit/app/apps/server/codecs/mappedData.spec.tsapps/meteor/app/apps/server/converters/codecs/users.tsapps/meteor/app/apps/server/converters/codecs/settings.tsapps/meteor/app/apps/server/converters/codecs/uploads.tsapps/meteor/app/apps/server/converters/roles.tsapps/meteor/app/apps/server/converters/visitors.tsapps/meteor/app/apps/server/converters/uploads.tsapps/meteor/app/apps/server/converters/codecs/departments.tsapps/meteor/app/apps/server/converters/videoConferences.tsapps/meteor/app/apps/server/converters/threads.tsapps/meteor/app/apps/server/converters/contacts.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/departments.tsapps/meteor/app/apps/server/converters/codecs/mappedData.tsapps/meteor/app/apps/server/converters/users.tsapps/meteor/app/apps/server/converters/codecs/rooms.tsapps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/settings.tsapps/meteor/app/apps/server/converters/rooms.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/app/apps/server/converters/codecs/videoConferences.tsapps/meteor/app/apps/server/converters/codecs/roles.tsapps/meteor/app/apps/server/converters/codecs/contacts.tsapps/meteor/app/apps/server/converters/codecs/index.tsapps/meteor/app/apps/server/converters/codecs/visitors.tsapps/meteor/tests/unit/app/apps/server/converters.edge.spec.tsapps/meteor/tests/unit/app/apps/server/codecs/mappedData.spec.tsapps/meteor/app/apps/server/converters/codecs/users.tsapps/meteor/app/apps/server/converters/codecs/settings.tsapps/meteor/app/apps/server/converters/codecs/uploads.tsapps/meteor/app/apps/server/converters/roles.tsapps/meteor/app/apps/server/converters/visitors.tsapps/meteor/app/apps/server/converters/uploads.tsapps/meteor/app/apps/server/converters/codecs/departments.tsapps/meteor/app/apps/server/converters/videoConferences.tsapps/meteor/app/apps/server/converters/threads.tsapps/meteor/app/apps/server/converters/contacts.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/departments.tsapps/meteor/app/apps/server/converters/codecs/mappedData.tsapps/meteor/app/apps/server/converters/users.tsapps/meteor/app/apps/server/converters/codecs/rooms.tsapps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/settings.tsapps/meteor/app/apps/server/converters/rooms.ts
📚 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/converters.edge.spec.tsapps/meteor/tests/unit/app/apps/server/codecs/mappedData.spec.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/converters.edge.spec.tsapps/meteor/tests/unit/app/apps/server/codecs/mappedData.spec.ts
🪛 LanguageTool
docs/proposals/apps-converters-zod/README.md
[style] ~268-~268: Try using a descriptive adverb here.
Context: ...eps its AsyncFieldMap loosely typed on purpose: its consumers (rooms, messages, `u...
(ON_PURPOSE_DELIBERATELY)
🔇 Additional comments (21)
apps/meteor/app/apps/server/converters/codecs/visitors.ts (1)
1-49: LGTM!apps/meteor/app/apps/server/converters/codecs/enums.ts (1)
26-40: 🩺 Stability & AvailabilityNo change needed. These codecs intentionally mirror the legacy enum helpers, and the existing enum tests already cover the supported string/undefined inputs and fallbacks.
> Likely an incorrect or invalid review comment.apps/meteor/app/apps/server/converters/codecs/index.ts (1)
2-9: LGTM!apps/meteor/app/apps/server/converters/codecs/roles.ts (1)
1-19: LGTM!apps/meteor/app/apps/server/converters/codecs/departments.ts (1)
1-56: LGTM!apps/meteor/app/apps/server/converters/codecs/settings.ts (1)
1-44: LGTM!apps/meteor/app/apps/server/converters/codecs/users.ts (1)
1-77: LGTM!apps/meteor/app/apps/server/converters/codecs/videoConferences.ts (1)
1-14: LGTM!apps/meteor/app/apps/server/converters/contacts.ts (1)
4-6: LGTM!Also applies to: 27-27, 39-39
apps/meteor/app/apps/server/converters/messages.ts (1)
9-9: LGTM!Also applies to: 69-69, 167-167, 376-376
apps/meteor/app/apps/server/converters/rooms.ts (1)
3-13: LGTM!Also applies to: 37-37, 48-52, 55-59, 73-73
apps/meteor/app/apps/server/converters/settings.ts (1)
4-6: LGTM!Also applies to: 20-20
apps/meteor/app/apps/server/converters/threads.ts (1)
9-9: LGTM!Also applies to: 54-54, 123-123, 134-134, 204-206
apps/meteor/app/apps/server/converters/uploads.ts (1)
4-13: LGTM!Also applies to: 33-33, 47-47
apps/meteor/app/apps/server/converters/departments.ts (1)
4-6: LGTM!Also applies to: 30-30, 44-44
apps/meteor/app/apps/server/converters/roles.ts (1)
5-7: LGTM!Also applies to: 20-20
apps/meteor/app/apps/server/converters/users.ts (1)
4-6: LGTM!Also applies to: 40-40
apps/meteor/app/apps/server/converters/videoConferences.ts (1)
4-6: LGTM!Also applies to: 26-30
apps/meteor/app/apps/server/converters/visitors.ts (1)
4-6: LGTM!Also applies to: 37-37, 51-51
apps/meteor/tests/unit/app/apps/server/converters.edge.spec.ts (1)
1-8: LGTM!Also applies to: 16-16, 20-62
docs/proposals/apps-converters-zod/README.md (1)
5-6: LGTM!Also applies to: 195-228, 252-274
d-gubert
marked this pull request as ready for review
July 16, 2026 20:25
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
![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 29 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…o Zod codecs First converters moved onto the codec pattern. Behaviour is unchanged — the Phase 0 golden snapshots still pass. - Add `createMappedCodec`: builds a codec from a string field map with the same `_unmappedProperties_` bucket semantics as `transformMappedData` (deep-clone isolation on decode, inverse rename + merge on encode). - Add entity codecs: `SettingCodec` (reuses `SettingTypeCodec`), `RoleCodec` (via `createMappedCodec`) and `VideoConferenceCodec` (pass-through clone). Endpoints use `z.custom` so no runtime validation is added yet, per the loose-validation decision. - Delegate `AppSettingsConverter`, `AppRolesConverter` and `AppVideoConferencesConverter` to the codecs; drop the now-unused `_convertTypeToApp` and the `transformMappedData` dependency in roles. - Unit-test `createMappedCodec` (rename, undefined handling, deep isolation, inverse, round-trip). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185hwJYnx3nzP4pcrYM2XXj
… codecs First bidirectional converters on the codec pattern. Behaviour unchanged — the Phase 0 golden snapshots (both directions) still pass. - Split the mapped-codec helper into reusable `mappedDecode` / `mappedEncode` transforms so a codec can pair a mapped decode with a bespoke encode. - `VisitorCodec` / `DepartmentCodec`: mapped decode (bucketed) + bespoke encode matching `convertAppVisitor` (status default, truthy-conditional fields, no `_updatedAt`/`activity` write-back) and `convertAppDepartment` (unconditional inverse), each merging `_unmappedProperties_`. - `UserCodec`: bespoke both ways via the shared enum codecs; the contextual invalid-status warning stays in the codec (it needs the user id/username), and the pre-existing `utfOffset` typo is preserved verbatim. - Delegate the three converter classes to their codecs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185hwJYnx3nzP4pcrYM2XXj
The enum codecs used `z.string()`/`z.string().optional()` endpoints, so `z.decode`/`z.encode` validated their input and threw on `null`/`undefined` — e.g. a user with `statusConnection: null` (Livechat guests, rocket.cat, bots, app-created users) threw instead of falling back to OFFLINE, and `convertToRocketChat` threw on a missing status/type. That broke the API integration suites. The legacy `_convert*` helpers never validated; they were pure transforms with fallbacks. Restore that by using `z.any()` endpoints so the switch logic handles every input exactly as before. Add regression tests for the null/undefined enum cases the golden fixtures didn't cover. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185hwJYnx3nzP4pcrYM2XXj
The regression spec passed fixtures with `as any`, which made TypeScript resolve `convertToApp`/`convertToRocketChat` to their first overload (`(user: undefined | null) => undefined`). Accessing `.statusConnection` /`.type` on the resulting `undefined` narrowed to `never`, failing the project typecheck (TS2339). Cast the fixtures to the real input types so the intended overload is selected and results stay typed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185hwJYnx3nzP4pcrYM2XXj
Async, cross-converter converters onto the codec pattern via factories closing over the orchestrator. Behaviour is unchanged — rooms.tests.ts (both directions, incl. isPartial) and the golden snapshots still pass. - Add `mappedDecodeAsync`: reproduces `transformMappedData`'s string- and (async) function-field branches plus the `_unmappedProperties_` bucket and deep-clone isolation, so the function-field converters can drop the `transformMappedData` dependency. - `createUploadsCodec(orch)`: async `decode` resolving room/user/visitor through their converters; sync `encode` reverse. Class delegates via `z.decodeAsync` / `z.encode`. - `createRoomCodec(orch)`: async `decode` (convertRoom — creator/visitor/ contact/department/closedBy/servedBy/responseBy/parentRoom via converters, secure fields, RoomType via the shared enum codec) and `encode` (the non-partial `convertAppRoom`, reading models directly). `convertRoomRaw` becomes a standalone `decodeRoomRaw`. The class keeps the `isPartial` variant, which the codec interface can't express. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185hwJYnx3nzP4pcrYM2XXj
…transformMappedData
Completes the RC->App migration: no converter imports `transformMappedData`
anymore. Behaviour is unchanged — messages.tests.js, rooms.tests.ts and the
golden snapshots (incl. contacts, both directions) still pass.
- Extend `mappedDecodeAsync` with the nested `{ from, map, list }` branch, so
it fully reproduces `transformMappedData` (string, function and nested/list
entries) including per-level `_unmappedProperties_` buckets.
- `ContactCodec`: `decode` clones (convertContact); `encode` maps every
attribute via the nested field map (convertAppContact). Class delegates via
`z.decodeAsync` / `z.encodeAsync`.
- messages & threads: replace the `transformMappedData` calls with
`mappedDecodeAsync`. The model/Random/`isMessageFromVisitor` usage stays in
the message class so the existing proxyquire-based tests keep stubbing it;
the cache, `isPartial` reverse and attachment sub-maps are untouched.
- Add nested/list unit tests for the helper.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0185hwJYnx3nzP4pcrYM2XXj
Every converter now maps through the codec helpers, so the legacy `transformMappedData` helper is no longer used anywhere. - Delete `app/apps/server/converters/transformMappedData.ts`. - Retire its (importer-located) spec; its coverage — explicit source deletion in a function, non-array `list` wrapping, and the empty-map case — is folded into the `mappedDecodeAsync` tests under `tests/unit/app/apps/server/codecs/`. - `cachedFunction` is kept: it still backs the messages/threads memoization that stayed in the class layer. - Document the messages/threads deviation (migrated via the shared `mappedDecodeAsync` helper rather than standalone codecs, to keep the proxyquire test harness working) and mark the plan phases complete. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185hwJYnx3nzP4pcrYM2XXj
Improves type declarativeness of the shared mapping helpers; no runtime change (type-only), all converter tests still pass. - `mappedDecode` / `mappedEncode` / `createMappedCodec` are now generic over the source document type. The field map is typed `FieldMap<Source>` (values constrained to `keyof Source`), so a misspelled/renamed source field is a compile error, and the result is the inferred `Decoded<Source, Map>` (renamed optional targets + typed `_unmappedProperties_` bucket) instead of `Record<string, any>`. `Source` defaults to a loose record so untyped call sites still work. - `visitors`/`departments`/`roles` pass their `Source` type to get the typo-safety; a `@ts-expect-error` test locks the constraint. - `mappedDecodeAsync` stays loose on the map (its consumers map dynamic livechat-only fields) but gains a `Result` type parameter, removing the `as unknown as Promise<…>` casts at uploads/rooms/messages/threads/contacts. - Document the type strategy in the proposal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185hwJYnx3nzP4pcrYM2XXj
- mappedDecodeAsync: guard nested non-list branch against undefined/null sources so absent/null sub-objects are omitted instead of throwing, and add an opt-in recursive `dropUnmapped` mode that omits _unmappedProperties_ at every level. - contacts: encode with `dropUnmapped` so hand-mapped app contacts never leak extra fields, matching convertAppContact's documented intent. - rooms codec: consume `contactId` and `v` before they reach the unmapped bucket; dedupe the `v` read via a `readVisitor` helper that removes it only after both visitor mappings have consumed it. - uploads codec: include `path` in the encode map so it survives round trips. - users codec: fall back from the legacy `utfOffset` typo to `utcOffset`. - docs: clarify that each affected codec boundary keeps one localized cast. - remove stale implementation comments; refresh golden/edge specs and add regression coverage for the guard, drop-unmapped mode, and utfOffset. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195KtUXPJHwhtamCHdgFZGg
Address two Hacktron DoS findings on the encode (Apps -> Rocket.Chat) path:
- rooms getVisitor: destructuring `visitorChannelInfo` threw when an app
supplied a visitor without channel info; default to `{}`.
- uploads encode: destructuring `upload.room` threw when the room was absent
(e.g. a deleted room omitted during decode); default to `{}` like the
adjacent user/visitor reads.
Add regression coverage for both.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0195KtUXPJHwhtamCHdgFZGg
…tors Address a second review round on the encode/decode converters: - rooms decode: stop consuming `v`/`contactId` from `_unmappedProperties_`. The legacy converter left both in the bucket, which round-trips them back on encode even when the visitor/contact cannot be resolved; removing them lost the original `contactId`/`v` on a RC->Apps->RC round trip. Restores the legacy behaviour (reverts the earlier bucket-consumption change). - mappedDecodeAsync: a non-list nested descriptor without a `map` previously recursed with `undefined` and crashed; now copy the cloned source verbatim. Restore the rooms golden snapshot and add regression coverage for the map-less nested descriptor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195KtUXPJHwhtamCHdgFZGg
d-gubert
force-pushed
the
claude/converters-zod-migration-p29du9
branch
from
July 17, 2026 14:12
783aa8a to
9c93d92
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/meteor/app/apps/server/converters/codecs/rooms.ts (1)
197-207: 📐 Maintainability & Code Quality | 🔵 TrivialResolve the two lookup TODOs.
Confirm whether the database existence checks are required, then remove the TODOs or track the decision externally. I can help open a follow-up issue.
🤖 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/codecs/rooms.ts` around lines 197 - 207, Resolve the TODOs above getContactId and getDepartment by confirming whether their database existence checks are required; remove both TODO comments once decided, or record the decision in an external follow-up issue if further investigation is needed.apps/meteor/app/apps/server/converters/codecs/enums.ts (2)
6-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove implementation narration across the migrated converters.
apps/meteor/app/apps/server/converters/codecs/enums.ts#L6-L19: remove migration-history and codec-mechanics comments.apps/meteor/app/apps/server/converters/codecs/roles.ts#L5-L10: remove the field-map implementation JSDoc.apps/meteor/app/apps/server/converters/codecs/rooms.ts#L10-L11: remove loose-record and migration-mechanics narration.apps/meteor/app/apps/server/converters/messages.ts#L12-L15: remove loose-record and fallback narration.As per coding guidelines, “Avoid code comments in the implementation.”
🤖 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/codecs/enums.ts` around lines 6 - 19, Remove the implementation-narration comments from the listed converter locations: delete the migration-history and codec-mechanics block in apps/meteor/app/apps/server/converters/codecs/enums.ts (lines 6-19), the field-map JSDoc in apps/meteor/app/apps/server/converters/codecs/roles.ts (lines 5-10), the loose-record and migration-mechanics comments in apps/meteor/app/apps/server/converters/codecs/rooms.ts (lines 10-11), and the loose-record and fallback comments in apps/meteor/app/apps/server/converters/messages.ts (lines 12-15); leave the implementation unchanged.Source: Coding guidelines
26-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
z.any()with explicit string-based schemas.These exported codecs accept arbitrary values, allowing non-strings to reach
.toUpperCase()and providing no useful inferred types. Explicitly model the legacyundefinedor falsy cases while retaining unknown-string passthrough.Verify the accepted legacy domains before narrowing the schemas.
Also applies to: 53-71, 78-94, 101-122
🤖 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/codecs/enums.ts` around lines 26 - 44, Update UserTypeCodec and the additionally referenced codecs to replace z.any() inputs/outputs with explicit string-based schemas, preserving accepted legacy undefined or falsy values after verifying each codec’s domain. Ensure decode cannot pass non-strings to toUpperCase, retains known enum mappings and unknown-string passthrough, and provides useful inferred types while keeping the existing encode behavior compatible.
🤖 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/codecs/rooms.ts`:
- Around line 198-203: Update getContactId to read the Apps contact’s id field
when resolving the contact, while preserving the existing enabled-contact lookup
and projected _id return behavior. Ensure app-created and modified room contacts
produce a contactId during encoding.
---
Nitpick comments:
In `@apps/meteor/app/apps/server/converters/codecs/enums.ts`:
- Around line 6-19: Remove the implementation-narration comments from the listed
converter locations: delete the migration-history and codec-mechanics block in
apps/meteor/app/apps/server/converters/codecs/enums.ts (lines 6-19), the
field-map JSDoc in apps/meteor/app/apps/server/converters/codecs/roles.ts (lines
5-10), the loose-record and migration-mechanics comments in
apps/meteor/app/apps/server/converters/codecs/rooms.ts (lines 10-11), and the
loose-record and fallback comments in
apps/meteor/app/apps/server/converters/messages.ts (lines 12-15); leave the
implementation unchanged.
- Around line 26-44: Update UserTypeCodec and the additionally referenced codecs
to replace z.any() inputs/outputs with explicit string-based schemas, preserving
accepted legacy undefined or falsy values after verifying each codec’s domain.
Ensure decode cannot pass non-strings to toUpperCase, retains known enum
mappings and unknown-string passthrough, and provides useful inferred types
while keeping the existing encode behavior compatible.
In `@apps/meteor/app/apps/server/converters/codecs/rooms.ts`:
- Around line 197-207: Resolve the TODOs above getContactId and getDepartment by
confirming whether their database existence checks are required; remove both
TODO comments once decided, or record the decision in an external follow-up
issue if further investigation is needed.
🪄 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: c6ac88fe-6dbb-419b-933f-0b80dceb59e8
📒 Files selected for processing (30)
apps/meteor/app/apps/server/converters/codecs/contacts.tsapps/meteor/app/apps/server/converters/codecs/departments.tsapps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/codecs/index.tsapps/meteor/app/apps/server/converters/codecs/mappedData.tsapps/meteor/app/apps/server/converters/codecs/roles.tsapps/meteor/app/apps/server/converters/codecs/rooms.tsapps/meteor/app/apps/server/converters/codecs/settings.tsapps/meteor/app/apps/server/converters/codecs/uploads.tsapps/meteor/app/apps/server/converters/codecs/users.tsapps/meteor/app/apps/server/converters/codecs/videoConferences.tsapps/meteor/app/apps/server/converters/codecs/visitors.tsapps/meteor/app/apps/server/converters/contacts.tsapps/meteor/app/apps/server/converters/departments.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/roles.tsapps/meteor/app/apps/server/converters/rooms.tsapps/meteor/app/apps/server/converters/settings.tsapps/meteor/app/apps/server/converters/threads.tsapps/meteor/app/apps/server/converters/transformMappedData.tsapps/meteor/app/apps/server/converters/uploads.tsapps/meteor/app/apps/server/converters/users.tsapps/meteor/app/apps/server/converters/videoConferences.tsapps/meteor/app/apps/server/converters/visitors.tsapps/meteor/tests/unit/app/apps/server/codecs/mappedData.spec.tsapps/meteor/tests/unit/app/apps/server/codecs/rooms.spec.tsapps/meteor/tests/unit/app/apps/server/converters.edge.spec.tsapps/meteor/tests/unit/app/apps/server/converters.golden.spec.tsapps/meteor/tests/unit/server/lib/import/transformMappedData.spec.tsdocs/proposals/apps-converters-zod/README.md
💤 Files with no reviewable changes (2)
- apps/meteor/app/apps/server/converters/transformMappedData.ts
- apps/meteor/tests/unit/server/lib/import/transformMappedData.spec.ts
🚧 Files skipped from review as they are similar to previous changes (23)
- apps/meteor/app/apps/server/converters/codecs/videoConferences.ts
- apps/meteor/app/apps/server/converters/codecs/settings.ts
- apps/meteor/tests/unit/app/apps/server/converters.edge.spec.ts
- apps/meteor/app/apps/server/converters/codecs/uploads.ts
- apps/meteor/app/apps/server/converters/codecs/index.ts
- apps/meteor/app/apps/server/converters/codecs/contacts.ts
- apps/meteor/app/apps/server/converters/videoConferences.ts
- apps/meteor/tests/unit/app/apps/server/codecs/mappedData.spec.ts
- apps/meteor/app/apps/server/converters/codecs/visitors.ts
- apps/meteor/app/apps/server/converters/roles.ts
- apps/meteor/app/apps/server/converters/codecs/users.ts
- apps/meteor/tests/unit/app/apps/server/codecs/rooms.spec.ts
- apps/meteor/app/apps/server/converters/codecs/departments.ts
- apps/meteor/app/apps/server/converters/uploads.ts
- apps/meteor/app/apps/server/converters/visitors.ts
- apps/meteor/app/apps/server/converters/users.ts
- apps/meteor/app/apps/server/converters/threads.ts
- apps/meteor/app/apps/server/converters/departments.ts
- apps/meteor/app/apps/server/converters/contacts.ts
- apps/meteor/app/apps/server/converters/codecs/mappedData.ts
- apps/meteor/app/apps/server/converters/settings.ts
- apps/meteor/tests/unit/app/apps/server/converters.golden.spec.ts
- apps/meteor/app/apps/server/converters/rooms.ts
📜 Review details
⚠️ CI failures not shown inline (6)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 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/apps/server/converters/codecs/roles.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/codecs/rooms.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/app/apps/server/converters/codecs/roles.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/codecs/rooms.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/app/apps/server/converters/codecs/roles.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/codecs/rooms.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/app/apps/server/converters/codecs/roles.tsapps/meteor/app/apps/server/converters/messages.tsapps/meteor/app/apps/server/converters/codecs/enums.tsapps/meteor/app/apps/server/converters/codecs/rooms.ts
🪛 LanguageTool
docs/proposals/apps-converters-zod/README.md
[style] ~268-~268: Try using a descriptive adverb here.
Context: ...eps its AsyncFieldMap loosely typed on purpose: its consumers (rooms, messages, `u...
(ON_PURPOSE_DELIBERATELY)
🔇 Additional comments (5)
docs/proposals/apps-converters-zod/README.md (1)
1-286: LGTM!apps/meteor/app/apps/server/converters/codecs/rooms.ts (2)
344-380: Relationship keys still leak into_unmappedProperties_.
vandcontactIdare not consumed by these function mappings, so the final unmapped merge can restore stale relationships over converted values. This concern was raised previously.
14-19: LGTM!Also applies to: 25-195, 207-213, 220-274, 282-343, 382-461
apps/meteor/app/apps/server/converters/codecs/roles.ts (1)
12-19: LGTM!apps/meteor/app/apps/server/converters/messages.ts (1)
1-10: LGTM!Also applies to: 16-41, 67-147, 149-150, 152-157, 163-212, 215-366, 368-376
ggazzo
previously approved these changes
Jul 21, 2026
- Drop `path` from the upload encode path: the legacy converter never wrote `path` back to Rocket.Chat (it is generated for Apps only and no longer persisted), so restoring it on encode was a behaviour regression. - Make the setting codec one-directional: the legacy settings converter had no `convertToRocketChat`, and an Apps-Engine setting cannot be turned back into a valid `ISetting` (required `env`/`blocked`/`sorter` are absent), so `encode` now throws instead of emitting an invalid, data-losing document. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EHqdzQxmGFrN6nTjfcRikA
d-gubert
force-pushed
the
claude/converters-zod-migration-p29du9
branch
from
July 24, 2026 22:44
e474761 to
db22f22
Compare
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
4 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
This PR migrates the apps converters from JavaScript to TypeScript and introduces Zod-based codec infrastructure for enum mappings. The changes preserve all existing behavior while improving type safety and maintainability.
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):decodeandencodeoperationsMigration approach:
_unmappedProperties_bucketing for forward compatibility_convert*ToApp/_convert*ToEnumhelpersIssue(s)
https://rocketchat.atlassian.net/browse/ARCH-2265
#41400
Part of the Zod-codec migration for apps converters.
Steps to test or reproduce
Run the new unit tests:
Verify existing apps functionality continues to work (apps installation, execution, field mapping)
Further comments
The golden snapshot tests serve as a regression suite during the codec migration. Each converter can be incrementally migrated to use the new codecs while these tests ensure behavior is preserved exactly. The
_unmappedProperties_bucket mechanism ensures forward compatibility when new fields are added to the Apps-Engine types.https://claude.ai/code/session_0185hwJYnx3nzP4pcrYM2XXj
Summary by CodeRabbit
createMappedCodec,mappedDecode*,mappedEncode) to handle nested/list field renames.