chore: add per-integration scriptTranspile flag to opt-out of Babel#40160
Open
chore: add per-integration scriptTranspile flag to opt-out of Babel#40160
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 |
🦋 Changeset detectedLatest commit: c217518 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
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 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughCentralizes integration script compilation in a new server utility that either validates syntax or transpiles via Babel, updates integration methods/validation to use it, introduces a per-integration Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)
86-105: Consider consolidating the database updates.The script compilation result is persisted in a separate
updateOnecall (lines 89-104) before the mainfindOneAndUpdate(lines 152-178). If the second operation fails, thescriptCompiled/scriptErrorchanges will persist while other fields remain unchanged. This two-phase update differs from the pattern invalidateOutgoingIntegration.tswhere compilation results are included in the integration data object.While this may preserve existing behavior, consider whether consolidating into a single atomic update would improve consistency.
🤖 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 - 105, The compilation result is being written via a separate Integrations.updateOne call after calling compileIntegrationScript(integration.script), which can lead to inconsistent state if the subsequent findOneAndUpdate fails; instead, run compileIntegrationScript first and then include either scriptCompiled or scriptError into the same update object passed to the existing findOneAndUpdate so the compilation fields are applied atomically with the rest of the integration changes (remove the standalone Integrations.updateOne block and merge its $set/$unset for scriptCompiled/scriptError into the findOneAndUpdate payload).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`:
- Around line 86-105: The compilation result is being written via a separate
Integrations.updateOne call after calling
compileIntegrationScript(integration.script), which can lead to inconsistent
state if the subsequent findOneAndUpdate fails; instead, run
compileIntegrationScript first and then include either scriptCompiled or
scriptError into the same update object passed to the existing findOneAndUpdate
so the compilation fields are applied atomically with the rest of the
integration changes (remove the standalone Integrations.updateOne block and
merge its $set/$unset for scriptCompiled/scriptError into the findOneAndUpdate
payload).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6562b514-14ac-4742-8e41-257cca4cf09e
📒 Files selected for processing (4)
apps/meteor/app/integrations/server/lib/compileIntegrationScript.tsapps/meteor/app/integrations/server/lib/validateOutgoingIntegration.tsapps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.tsapps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- 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/lib/validateOutgoingIntegration.tsapps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.tsapps/meteor/app/integrations/server/lib/compileIntegrationScript.tsapps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
🧠 Learnings (5)
📓 Common learnings
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: 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: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
📚 Learning: 2026-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/lib/validateOutgoingIntegration.tsapps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.tsapps/meteor/app/integrations/server/lib/compileIntegrationScript.tsapps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.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/lib/validateOutgoingIntegration.tsapps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.tsapps/meteor/app/integrations/server/lib/compileIntegrationScript.tsapps/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/lib/validateOutgoingIntegration.tsapps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.tsapps/meteor/app/integrations/server/lib/compileIntegrationScript.tsapps/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 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/lib/validateOutgoingIntegration.tsapps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.tsapps/meteor/app/integrations/server/lib/compileIntegrationScript.tsapps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
🔇 Additional comments (10)
apps/meteor/app/integrations/server/lib/compileIntegrationScript.ts (5)
1-5: LGTM!Imports are appropriate for the module's functionality.
6-24: LGTM!The environment variable parsing is robust, and the deprecation/opt-in messages at module load time are appropriate for guiding admins through the migration.
37-45: LGTM!The function signature and return type correctly match the
scriptErrortype contract frompackages/core-typings/src/IIntegration.ts.
47-61: LGTM!The IIFE wrapper
(function(){...})correctly validates that the script is syntactically valid as a function body, and onlySyntaxErroris caught as expected for syntax validation.
63-82: LGTM!The Babel configuration and error handling are appropriate. The fallback to the original script when
result?.codeis undefined preserves existing behavior.apps/meteor/app/integrations/server/lib/validateOutgoingIntegration.ts (2)
6-6: LGTM!Import of the centralized helper is correct.
180-182: LGTM!The direct assignment pattern works correctly because
compileIntegrationScriptreturns mutually exclusive properties - eitherscriptorerrorwill be defined. This is a valid simplification over explicit conditional checks.apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts (2)
12-12: LGTM!Import of the centralized helper is correct.
111-118: LGTM!The conditional handling correctly sets
scriptErroron compilation failure andscriptCompiledon success, with proper cleanup of the opposite field.apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)
10-10: LGTM!Import of the centralized helper is correct.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40160 +/- ##
===========================================
- Coverage 70.19% 70.18% -0.02%
===========================================
Files 3280 3281 +1
Lines 116852 116915 +63
Branches 20666 20739 +73
===========================================
+ Hits 82020 82052 +32
- Misses 31541 31574 +33
+ Partials 3291 3289 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
chore/optional-integration-transpile
branch
from
5c0cd7b to
9af44a7
Compare
ggazzo
force-pushed
the
chore/optional-integration-transpile
branch
from
9af44a7 to
f09729e
Compare
ggazzo
changed the title
Integration scripts are transpiled with @babel/core + @babel/preset-env
before being stored. In 9.0.0, this transpilation will be removed
entirely (scripts run as-is in isolated-vm's modern V8).
This commit adds a per-integration `scriptTranspile` flag (default
`true`) so admins can opt-out one integration at a time, test their
scripts for strict-mode compatibility, and migrate gradually before
upgrading to 9.0.0.
Changes:
- New apps/meteor/app/integrations/server/lib/compileIntegrationScript
helper: takes `{ transpile: boolean }` and either runs Babel (legacy)
or validates syntax with Node's built-in vm.Script (9.0.0 behavior).
- Add `scriptTranspile?: boolean` to IIncomingIntegration and
IOutgoingIntegration. Field is documented as deprecated.
- Extend rest-typings Create and Update schemas to accept the flag.
- Add/Update methods (incoming + outgoing) now pass
`integration.scriptTranspile !== false` to the compiler and persist
the flag on the document.
Known strict-mode breaking patterns (documented in 9.0.0 changeset)
when `scriptTranspile: false`:
- Implicit globals: msg = x → const msg = x
- this in nested functions: this === undefined instead of globalThis
- arguments.callee: TypeError
- Octal literals: 0777 → 0o777
- Duplicate parameter names
ggazzo
force-pushed
the
chore/optional-integration-transpile
branch
from
f09729e to
c217518
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
1 participant
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.
Summary
Prepares the ground for the 9.0.0 breaking change (removal of Babel transpilation for webhook integration scripts) by letting admins opt-out per integration.
What changes
compileIntegrationScript(script, { transpile })helper (apps/meteor/app/integrations/server/lib/compileIntegrationScript.ts): centralizes compile-or-validate logic. Whentranspile: true(default), runs@babel/core + @babel/preset-env. Whentranspile: false, validates with Node's built-invm.Scriptand stores the script as-is — matching the 9.0.0 default.scriptTranspile?: booleanfield onIIncomingIntegrationandIOutgoingIntegration(defaulttrue). Exposed via rest-typings for Create and Update endpoints.integration.scriptTranspile !== falseto the compiler and persist the flag on the document.Why per-integration (not global env var)
A single workspace can have dozens of webhook integrations with scripts written by different teams over years. A global env var would be all-or-nothing. With the per-integration flag, admins can:
scriptTranspile: falsein stagingscriptTranspile: falseset on every integrationAPI usage
Patterns that break with
scriptTranspile: falsemsg = x(implicit global in class method)const msg = xfunction f() { this.JSON }(this in nested function)arguments.callee0777(octal literal)0o777function(a, a)(duplicate params)Test plan
scriptTranspileunset): Babel transpilation works as before.scriptTranspile: false: scripts stored as-is, syntax errors caught, runtime executes in strict mode.Summary by CodeRabbit
New Features
Bug Fixes