Skip to content

chore!: stop transpiling webhook integration scripts with Babel#40142

Draft
ggazzo wants to merge 275 commits intorelease-9.0.0from
perf/integration-scripts-no-babel
Draft

chore!: stop transpiling webhook integration scripts with Babel#40142
ggazzo wants to merge 275 commits intorelease-9.0.0from
perf/integration-scripts-no-babel

Conversation

@ggazzo
Copy link
Copy Markdown
Member

@ggazzo ggazzo commented Apr 13, 2026

Summary

Remove @babel/core + @babel/preset-env from webhook integration script compilation. Scripts are now stored as-is and executed natively by isolated-vm (V8). Syntax validation at save time uses Node's built-in vm.Script.

Target: 9.0.0 — this is a breaking change for scripts that rely on sloppy-mode behaviors.

Breaking changes to review during upgrade

Class method bodies are always in strict mode per ES2015. The previous Babel transpilation converted class to ES5 function prototypes, which are NOT strict — masking these issues. After this change, scripts must follow strict-mode rules:

1. Implicit globals (HIGH risk)

  class Script {
    process_incoming_request({ request }) {
-     msg = buildMessage(request.content);  // ReferenceError in strict
+     const msg = buildMessage(request.content);
      return { content: { text: msg.text } };
    }
  }

2. this in nested regular functions (HIGH risk)

  class Script {
    process_incoming_request({ request }) {
-     function helper() {
-       return this.JSON.stringify(request);  // this === undefined in strict
-     }
+     const helper = () => {
+       return JSON.stringify(request);  // arrow fn captures outer scope
+     };
      return { content: { text: helper() } };
    }
  }

3. arguments.callee (MEDIUM risk)

  class Script {
    process_incoming_request({ request }) {
-     const factorial = function(n) {
-       return n <= 1 ? 1 : n * arguments.callee(n - 1);  // TypeError
-     };
+     const factorial = function f(n) {
+       return n <= 1 ? 1 : n * f(n - 1);  // named expression
+     };
    }
  }

4. Octal literals (LOW risk)

- const perms = 0777;   // SyntaxError in strict
+ const perms = 0o777;  // ES2015 octal

5. Duplicate parameter names (LOW risk)

- function merge(a, a) { ... }  // SyntaxError in strict
+ function merge(a, b) { ... }

Changes

  • New apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts: wraps new vm.Script(...) from Node's built-in vm module. Syntax errors are surfaced at save time through the existing scriptError field.
  • validateOutgoingIntegration.ts, addIncomingIntegration.ts, updateIncomingIntegration.ts: swap transformSync(@babel/core) for validateScriptSyntax.
  • apps/meteor/package.json: drop @babel/core and @babel/preset-env. @babel/runtime stays (Meteor-compiled packages use it).
  • Fix e2e test incoming-integrations.ts: msg = buildMessage(...)const msg = buildMessage(...).
  • Changeset marked as major.

Expected image impact

~50 MB reduction (entire @babel/core + @babel/preset-env + plugin-transform-* + helper-* + traverse + generator tree removed from programs/server/npm/node_modules/).

Upgrade recommendation

Admins should review existing webhook scripts (Admin → Integrations) before upgrading to 9.0.0. Any script using class Script { ... } that contains the patterns above needs to be updated. The webhook editor could show a warning banner during the 8.x→9.0 transition period.

Test plan

  • TypeScript / lint pass.
  • POST /integrations.createshould return scriptCompiled and no scriptError passes.
  • POST /hooks/:id/:token with proper const declarations: returns 200.
  • Integration with deliberate syntax error: scriptError populated, no scriptCompiled.
  • Existing integrations continue to run (their scriptCompiled is older Babel output — valid JS that V8 runs unchanged).

ergot-rp and others added 30 commits February 25, 2026 21:39
Co-authored-by: erik <erik.gothe@fotbollsdata.se>
Co-authored-by: gabriellsh <40830821+gabriellsh@users.noreply.github.com>
Co-authored-by: Douglas Fabris <devfabris@gmail.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
…er as italic (#38981)

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
Co-authored-by: Matheus Cardoso <matheus@cardo.so>
Co-authored-by: Douglas Fabris <devfabris@gmail.com>
Co-authored-by: gabriellsh <40830821+gabriellsh@users.noreply.github.com>
Co-authored-by: Douglas Fabris <devfabris@gmail.com>
Co-authored-by: Matheus Cardoso <matheus@cardo.so>
…points (#39094)

Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dodaa08 <dodaa08@users.noreply.github.com>
#39014)

Co-authored-by: Kaustubh <kaus@localhost.localdomain>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Douglas Fabris <devfabris@gmail.com>
Release 8.2.0
ricardogarim and others added 11 commits April 9, 2026 17:10
Co-authored-by: Douglas Fabris <devfabris@gmail.com>
Co-authored-by: erik <erik.gothe@fotbollsdata.se>
Co-authored-by: juliajforesti <juliajforesti@gmail.com>
- Add yarn patches for react-aria and react-stately that slim their
  barrel entry points to only re-export the sub-packages actually used
- Replace barrel imports with direct sub-package imports across
  apps/meteor and workspace packages (gazzodown, ui-client,
  ui-contexts, ui-voip)
- Add yarn patch for zod that removes `export * as locales` from all
  barrel entry points (classic, core, mini — ESM and CJS)
- Only English locale is used; other 49 locale files were dead weight
  in the bundle
@ggazzo ggazzo requested a review from a team as a code owner April 13, 2026 20:54
@dionisio-bot
Copy link
Copy Markdown
Contributor

dionisio-bot bot commented Apr 13, 2026

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is targeting the wrong base branch. It should target 9.0.0, but it targets 8.4.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 13, 2026

🦋 Changeset detected

Latest commit: 7b70013

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 41 packages
Name Type
@rocket.chat/meteor Major
@rocket.chat/core-typings Major
@rocket.chat/rest-typings Major
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/gazzodown Major
@rocket.chat/http-router Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Major
@rocket.chat/ui-client Major
@rocket.chat/ui-contexts Major
@rocket.chat/ui-voip Major
@rocket.chat/web-ui-registration Major
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/abac Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/license Patch
@rocket.chat/media-calls Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Major
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch
@rocket.chat/server-fetch Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 13, 2026

Walkthrough

Replaces Babel-based transpilation of webhook integration scripts with a new SWC-based syntax/compile validation flow via a new validateScriptSyntax utility and updates integration methods to use its results; also updates runtime dependencies to remove Babel packages and add @swc/core.

Changes

Cohort / File(s) Summary
Changeset
.changeset/integration-scripts-no-babel.md
Adds a changeset entry documenting the change from Babel to SWC for integration script handling and resulting container image dependency adjustments.
New Syntax Validation Utility
apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts
Adds validateScriptSyntax(script) which compiles scripts synchronously with @swc/core targeting ES5 (loose mode), strips emitted "use strict", and returns either { script } or { error: { name, message, stack } }.
Integration validation logic
apps/meteor/app/integrations/server/lib/validateOutgoingIntegration.ts, apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts, apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
Removes Babel transformSync transpilation and related try/catch shaping. Replaces compilation flow with validateScriptSyntax calls; updates assignments to integrationData.scriptCompiled / integrationData.scriptError based on returned { script, error }.
Runtime dependencies
apps/meteor/package.json
Removes @babel/core and @babel/preset-env from runtime deps and adds @swc/core while keeping @babel/runtime.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

type: chore

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: replacing Babel transpilation with syntax validation for webhook integration scripts.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ggazzo ggazzo added this to the 8.4.0 milestone Apr 13, 2026
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 6 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts">

<violation number="1" location="apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts:21">
P1: Syntax validation uses a wrapper that does not match runtime compilation context, causing incorrect accept/reject results.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 13, 2026

Codecov Report

❌ Patch coverage is 84.04908% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.20%. Comparing base (6d98be1) to head (7b70013).
⚠️ Report is 178 commits behind head on release-9.0.0.

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                @@
##           release-9.0.0   #40142      +/-   ##
=================================================
- Coverage          70.79%   70.20%   -0.59%     
=================================================
  Files               3142     3280     +138     
  Lines             108949   116814    +7865     
  Branches           19620    20676    +1056     
=================================================
+ Hits               77131    82012    +4881     
- Misses             29820    31524    +1704     
- Partials            1998     3278    +1280     
Flag Coverage Δ
e2e 59.71% <100.00%> (-0.63%) ⬇️
e2e-api 46.57% <ø> (-2.54%) ⬇️
unit 71.03% <83.95%> (-0.88%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ggazzo ggazzo force-pushed the perf/integration-scripts-no-babel branch from 8f58324 to b38bbcb Compare April 13, 2026 22:16
@ggazzo ggazzo changed the title chore: stop transpiling webhook integration scripts with Babel chore: swap Babel for SWC in webhook integration script compilation Apr 13, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)

87-88: Remove inline implementation comments in this block.

Please remove these inline comments. As per coding guidelines, "Avoid code comments in the implementation".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`
around lines 87 - 88, Remove the inline implementation comments in the block
that contains the "isolated-vm embeds modern V8 and runs the script natively, so
no transpilation is needed. Syntax is still validated at save time." text inside
updateIncomingIntegration.ts (within the updateIncomingIntegration handling
code); delete that comment line(s) so the implementation contains no inline
explanatory comments per the coding guideline.
apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts (1)

105-107: Drop inline implementation comments here.

Please remove these comments from implementation code. As per coding guidelines, "Avoid code comments in the implementation".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts`
around lines 105 - 107, Remove the inline implementation comments inside the
addIncomingIntegration handler that explain script validation and sandbox
behavior (the two lines starting with "Validate the script syntax..." and
"isolated-vm embeds modern V8..."), leaving only code and higher-level
documentation elsewhere; locate the block in addIncomingIntegration
(incoming/addIncomingIntegration.ts) around the script validation logic and
delete those comment lines so the implementation contains no inline explanatory
comments.
apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts (1)

3-20: Remove implementation comments in this file.

Please remove the block/inline comments in implementation code and keep rationale in PR/ADR docs instead. As per coding guidelines, "Avoid code comments in the implementation".

Also applies to: 35-37

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts` around lines
3 - 20, Remove the implementation comments in this file: delete the large
top-block comment describing compilation rationale and any inline/block comments
around the transform/validation logic (including the notes referencing
scriptCompiled and scriptError and the comments at lines ~35-37). Keep only
minimal JSDoc/signature comments if needed (e.g., function name
validateScriptSyntax), and move the detailed rationale into PR/ADR docs as per
guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts`:
- Around line 25-40: The validateScriptSyntax function currently transpiles user
code via transformSync (using jsc.target/jsc.loose) and then strips "use strict"
via a global regex, which mutates valid scripts; instead, replace the
transformSync-based flow in validateScriptSyntax with a syntax-only validation
using node:vm.Script (or new vm.Script) to parse/compile the input without
transforming it, remove the code.replace(/"use strict".../) step entirely, and
return the original script unchanged on success (throw or propagate the
vm.Script error on parse failure) so that functions/methods referencing
transformSync, jsc.target/jsc.loose, and the regex replacement are removed or
disabled.

In
`@apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`:
- Around line 86-106: The code currently writes scriptCompiled/scriptError to
the DB immediately after validateScriptSyntax (inside the isFrozen/scriptEnabled
branch) which can leave partial state if later validations fail; instead, call
validateScriptSyntax(integration.script) and keep the resulting script and error
in local variables and build a single atomic update object (with $set/$unset
entries for scriptCompiled and scriptError as needed) and apply it once together
with the other validations using the final findOneAndUpdate (or a single
Integrations.updateOne) after all channel/user validations succeed; update
references: validateScriptSyntax, integration.script, scriptCompiled,
scriptError, integrationId, and remove the immediate Integrations.updateOne
calls so the DB write is deferred and atomic.

---

Nitpick comments:
In `@apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts`:
- Around line 3-20: Remove the implementation comments in this file: delete the
large top-block comment describing compilation rationale and any inline/block
comments around the transform/validation logic (including the notes referencing
scriptCompiled and scriptError and the comments at lines ~35-37). Keep only
minimal JSDoc/signature comments if needed (e.g., function name
validateScriptSyntax), and move the detailed rationale into PR/ADR docs as per
guidelines.

In
`@apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts`:
- Around line 105-107: Remove the inline implementation comments inside the
addIncomingIntegration handler that explain script validation and sandbox
behavior (the two lines starting with "Validate the script syntax..." and
"isolated-vm embeds modern V8..."), leaving only code and higher-level
documentation elsewhere; locate the block in addIncomingIntegration
(incoming/addIncomingIntegration.ts) around the script validation logic and
delete those comment lines so the implementation contains no inline explanatory
comments.

In
`@apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`:
- Around line 87-88: Remove the inline implementation comments in the block that
contains the "isolated-vm embeds modern V8 and runs the script natively, so no
transpilation is needed. Syntax is still validated at save time." text inside
updateIncomingIntegration.ts (within the updateIncomingIntegration handling
code); delete that comment line(s) so the implementation contains no inline
explanatory comments per the coding guideline.
🪄 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: cf1e44ca-052c-40b6-8e3e-2a5d3241e6cb

📥 Commits

Reviewing files that changed from the base of the PR and between 8f58324 and b38bbcb.

📒 Files selected for processing (6)
  • .changeset/integration-scripts-no-babel.md
  • apps/meteor/app/integrations/server/lib/validateOutgoingIntegration.ts
  • apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts
  • apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts
  • apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
  • apps/meteor/package.json
✅ Files skipped from review due to trivial changes (2)
  • .changeset/integration-scripts-no-babel.md
  • apps/meteor/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/meteor/app/integrations/server/lib/validateOutgoingIntegration.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation

Files:

  • apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts
  • apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
  • apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.

Applied to files:

  • apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts
  • apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
  • apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.

Applied to files:

  • apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts
  • apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.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/app/integrations/server/methods/incoming/addIncomingIntegration.ts
  • apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
  • apps/meteor/app/integrations/server/lib/validateScriptSyntax.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/integrations/server/methods/incoming/addIncomingIntegration.ts
  • apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
  • apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.

Applied to files:

  • apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests

Applied to files:

  • apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.

Applied to files:

  • apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.

Applied to files:

  • apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts
🔇 Additional comments (1)
apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts (1)

13-14: Good centralization of script validation result handling.

Using validateScriptSyntax and explicitly mapping success/error to scriptCompiled/scriptError keeps behavior clear and consistent.

Also applies to: 114-121

Comment on lines +25 to +40
const { code } = transformSync(script, {
jsc: {
target: 'es5',
parser: { syntax: 'ecmascript' },
loose: true,
},
isModule: false,
minify: false,
});

// SWC emits `"use strict"` directives for the IIFE wrapping the class
// transform. Remove them so the transpiled methods keep the sloppy-mode
// semantics the previous Babel-based flow provided.
const compiled = code.replace(/"use strict";?\s*/g, '');

return { script: compiled };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🌐 Web query:

From official docs: does Node.js vm.Scripttransform source code or only compile/validate it? Also, does@swc/core transformSyncwithjsc.target: "es5"andjsc.loose: true perform semantic transpilation (not syntax-only validation)?

💡 Result:

Node.js vm.Script only compiles and validates the source code into a precompiled script (V8 UnboundScript); it does not transform or modify the source code AST or semantics. The constructor "compiles code but does not run it" and produces a cached representation for execution in contexts, without any syntax transformation like transpilation to older targets. @swc/core transformSync with jsc.target: "es5" and jsc.loose: true performs semantic transpilation. The target: "es5" enables transforms that rewrite modern syntax (e.g., classes, arrow functions, spread) into equivalent ES5 polyfills/helpers, changing code semantics to match older environments. loose: true enables faster, less spec-compliant implementations with different semantics (e.g., assumptions allowing incorrect output for some inputs like Set spreading), but still transpiles rather than syntax-only validation.

Citations:


validateScriptSyntax transpiles user code instead of validating it, corrupting valid scripts and breaking semantics.

The current implementation uses transformSync with jsc.target: 'es5' and jsc.loose: true, which performs semantic transpilation, not syntax-only validation. Additionally, the global regex /"use strict";?\s*/g on line 38 can corrupt legitimate string literals—for example, const s = "use strict"; becomes const s = ;, a syntax error.

Use node:vm Script for validation without transformation, and return the original script unchanged.

Proposed fix
-import { transformSync } from '@swc/core';
+import { Script } from 'node:vm';

 export function validateScriptSyntax(
 	script: string,
 ): { script: string; error?: undefined } | { script?: undefined; error: Pick<Error, 'name' | 'message' | 'stack'> } {
 	try {
-		const { code } = transformSync(script, {
-			jsc: {
-				target: 'es5',
-				parser: { syntax: 'ecmascript' },
-				loose: true,
-			},
-			isModule: false,
-			minify: false,
-		});
-
-		const compiled = code.replace(/"use strict";?\s*/g, '');
-
-		return { script: compiled };
+		new Script(script);
+		return { script };
 	} catch (e) {
 		if (e instanceof Error) {
 			const { name, message, stack } = e;
 			return { error: { name, message, stack } };
 		}
 		throw e;
 	}
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const { code } = transformSync(script, {
jsc: {
target: 'es5',
parser: { syntax: 'ecmascript' },
loose: true,
},
isModule: false,
minify: false,
});
// SWC emits `"use strict"` directives for the IIFE wrapping the class
// transform. Remove them so the transpiled methods keep the sloppy-mode
// semantics the previous Babel-based flow provided.
const compiled = code.replace(/"use strict";?\s*/g, '');
return { script: compiled };
new Script(script);
return { script };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/meteor/app/integrations/server/lib/validateScriptSyntax.ts` around lines
25 - 40, The validateScriptSyntax function currently transpiles user code via
transformSync (using jsc.target/jsc.loose) and then strips "use strict" via a
global regex, which mutates valid scripts; instead, replace the
transformSync-based flow in validateScriptSyntax with a syntax-only validation
using node:vm.Script (or new vm.Script) to parse/compile the input without
transforming it, remove the code.replace(/"use strict".../) step entirely, and
return the original script unchanged on success (throw or propagate the
vm.Script error on parse failure) so that functions/methods referencing
transformSync, jsc.target/jsc.loose, and the regex replacement are removed or
disabled.

Comment on lines +86 to 106
if (!isFrozen && integration.scriptEnabled === true && integration.script && integration.script.trim() !== '') {
// isolated-vm embeds modern V8 and runs the script natively, so no
// transpilation is needed. Syntax is still validated at save time.
const { script, error } = validateScriptSyntax(integration.script);
if (error) {
await Integrations.updateOne(
{ _id: integrationId },
{
$set: { scriptError: error },
$unset: { scriptCompiled: 1 as const },
},
);
} else {
await Integrations.updateOne(
{ _id: integrationId },
{
$set: { scriptCompiled: script },
$unset: { scriptError: 1 as const },
},
);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Avoid pre-validation DB writes; this can persist partial state.

Line 91/Line 99 write scriptCompiled/scriptError before channel/user validations complete. If a later validation throws, the method fails but those fields remain changed (partial update). Build script $set/$unset in memory and apply once in the final findOneAndUpdate.

Proposed fix sketch
-	if (!isFrozen && integration.scriptEnabled === true && integration.script && integration.script.trim() !== '') {
-		const { script, error } = validateScriptSyntax(integration.script);
-		if (error) {
-			await Integrations.updateOne(
-				{ _id: integrationId },
-				{ $set: { scriptError: error }, $unset: { scriptCompiled: 1 as const } },
-			);
-		} else {
-			await Integrations.updateOne(
-				{ _id: integrationId },
-				{ $set: { scriptCompiled: script }, $unset: { scriptError: 1 as const } },
-			);
-		}
-	}
+	let scriptSet: Partial<IIntegration> = {};
+	let scriptUnset: Record<string, 1> = {};
+	if (!isFrozen && integration.scriptEnabled === true && integration.script && integration.script.trim() !== '') {
+		const { script, error } = validateScriptSyntax(integration.script);
+		if (error) {
+			scriptSet = { scriptError: error };
+			scriptUnset = { scriptCompiled: 1 };
+		} else {
+			scriptSet = { scriptCompiled: script };
+			scriptUnset = { scriptError: 1 };
+		}
+	}
...
-	const updatedIntegration = await Integrations.findOneAndUpdate(
+	const updatedIntegration = await Integrations.findOneAndUpdate(
 		{ _id: integrationId },
 		{
-			$set: {
+			$set: {
 				...,
+				...scriptSet,
 			},
+			...(Object.keys(scriptUnset).length ? { $unset: scriptUnset } : {}),
 		},
 		{ returnDocument: 'after' },
 	);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`
around lines 86 - 106, The code currently writes scriptCompiled/scriptError to
the DB immediately after validateScriptSyntax (inside the isFrozen/scriptEnabled
branch) which can leave partial state if later validations fail; instead, call
validateScriptSyntax(integration.script) and keep the resulting script and error
in local variables and build a single atomic update object (with $set/$unset
entries for scriptCompiled and scriptError as needed) and apply it once together
with the other validations using the final findOneAndUpdate (or a single
Integrations.updateOne) after all channel/user validations succeed; update
references: validateScriptSyntax, integration.script, scriptCompiled,
scriptError, integrationId, and remove the immediate Integrations.updateOne
calls so the DB write is deferred and atomic.

@ggazzo ggazzo marked this pull request as draft April 13, 2026 22:32
BREAKING CHANGE: Webhook integration scripts are no longer transpiled
with @babel/core + @babel/preset-env before being stored. Scripts now
run as-is inside isolated-vm (modern V8).

This means class method bodies are in strict mode per the ES2015 spec.
Scripts that relied on sloppy-mode behaviors provided by the previous
Babel transpilation must be updated:

- Implicit globals: `msg = x` → `const msg = x`
- this in nested functions: `function f() { this.JSON }` → use arrow
  function or pass dependency explicitly
- arguments.callee → use named function expression
- Octal literals: 0777 → 0o777
- Duplicate parameter names: function(a, a) → rename

Changes:
- New validateScriptSyntax helper using Node's built-in vm.Script for
  syntax validation without transpilation.
- Drop @babel/core and @babel/preset-env from apps/meteor dependencies.
  @babel/runtime stays (used by Meteor-compiled packages).
- Fix e2e test that used implicit global (msg → const msg).
@ggazzo ggazzo force-pushed the perf/integration-scripts-no-babel branch from b38bbcb to 7b70013 Compare April 14, 2026 03:24
@ggazzo ggazzo modified the milestones: 8.4.0, 9.0.0 Apr 14, 2026
@ggazzo ggazzo changed the title chore: swap Babel for SWC in webhook integration script compilation chore: stop transpiling webhook integration scripts with Babel Apr 14, 2026
@ggazzo ggazzo changed the title chore: stop transpiling webhook integration scripts with Babel chore!: stop transpiling webhook integration scripts with Babel Apr 14, 2026
@ggazzo ggazzo changed the base branch from develop to release-9.0.0 April 14, 2026 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.