fix: Webhook script compilation failure #38048
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughScript compilation now fails fast for incoming integrations (throws Meteor.Error on compile failure) and channel processing was changed to derive and use a non-mutated Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Method as IncomingMethod
participant Compiler
participant DB
Client->>IncomingMethod: call add/updateIncomingIntegration(payload)
IncomingMethod->>IncomingMethod: validate permissions & params
IncomingMethod->>Compiler: compile(payload.script)
alt compilation succeeds
Compiler-->>IncomingMethod: compiledScript
IncomingMethod->>DB: insert/update integration (include scriptCompiled, $unset scriptError)
DB-->>IncomingMethod: ack
IncomingMethod-->>Client: success (integration saved)
else compilation fails
Compiler-->>IncomingMethod: compile error
IncomingMethod-->>Client: throw Meteor.Error('error-invalid-script')
Note right of DB: No persistence of invalid script occurs
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts">
<violation number="1" location="apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:85">
P0: Critical: This code block has been moved outside the function scope to module level. The variables `scriptEngine`, `integrationId`, and `integration` are only defined inside the `updateIncomingIntegration` function. This will cause a `ReferenceError` at module load time and break the entire integration update functionality.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts (1)
118-127: Consider removing redundant field assignments.The assignments to
integrationData.scriptCompiledandintegrationData.scriptErroron lines 119-120 are effectively dead code since theMeteor.Erroris thrown immediately after, preventing the integration from being saved. The error handling logic correctly surfaces compilation errors to the UI.🔎 Proposed simplification
} catch (e) { - integrationData.scriptCompiled = undefined; - integrationData.scriptError = e instanceof Error ? _.pick(e, 'name', 'message', 'stack') : undefined; - // Surfacing the error to the UI to prevent saving a broken integration throw new Meteor.Error( 'error-invalid-script', `Compilation Error: ${e instanceof Error ? e.message : 'Unknown error'}`, ); }apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)
85-125: Indentation is inconsistent within the new block.The code block starting at line 85 (
const isFrozen = ...) and theif (!isFrozen)block (lines 87-125) appear to have inconsistent indentation compared to the rest of the function. The function body uses tabs, but this block appears to use different spacing.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.tsapps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.tsapps/meteor/package.json
🧰 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.tsapps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
🧬 Code graph analysis (1)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)
packages/models/src/index.ts (1)
Integrations(157-157)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
Outdated
Show resolved
Hide resolved
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
Outdated
Show resolved
Hide resolved
…redundant babel plugin
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts">
<violation number="1" location="apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:183">
P3: Dead code: Since compilation errors throw before reaching this update, `scriptError` will always be falsy here. This spread will always produce an empty object.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)
198-201: Wrong method name in error details.The error references
updateOutgoingIntegrationbut this is theupdateIncomingIntegrationmethod.🔎 Proposed fix
if (!this.userId) { throw new Meteor.Error('error-invalid-user', 'Invalid user', { - method: 'updateOutgoingIntegration', + method: 'updateIncomingIntegration', }); }
🧹 Nitpick comments (4)
apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts (1)
128-131: Usesliceinstead of deprecatedsubstr; considerconstfor loop variable.
substris deprecated. Additionally, sincechannelis no longer reassigned in the loop body, it can be declared asconst.🔎 Proposed fix
- for await (let channel of channels) { + for await (const channel of channels) { let record; const channelType = channel[0]; - const channelName = channel.substr(1); + const channelName = channel.slice(1);apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (3)
86-108: Compilation error handling is correct, but lines 99-101 are dead code.The throw at line 106 ensures the function exits before reaching
findOneAndUpdate, which is the correct behavior. However, settingscriptErrorin the catch block (lines 99-101) serves no purpose since we throw immediately after—the value is never used.🔎 Proposed simplification
} catch (e) { - scriptCompiled = undefined; - if (e instanceof Error) { - const { name, message, stack } = e; - scriptError = { name, message, stack }; - } - - // We throw the error to notify the UI. Because we are using findOneAndUpdate - // later, the error state is NOT persisted if we throw here, keeping the DB clean. + // Throw to notify the UI; DB is not updated since we exit before findOneAndUpdate. throw new Meteor.Error('error-invalid-script', `Compilation Error: ${e instanceof Error ? e.message : 'Unknown error'}`); }
110-112: Considerconstfor loop variable.Since
channelis no longer reassigned in the loop body, it can be declared asconst.🔎 Proposed fix
- for await (let channel of channels) { + for await (const channel of channels) { const channelType = channel[0]; const channelName = channel.slice(1);
181-184: Line 183 is dead code—scriptErroris never set when reaching this point.Since the function throws when compilation fails (line 106),
scriptErroris alwaysundefinedhere. The$unsetforscriptCompiledbased onscriptErrorwill never execute.🔎 Proposed simplification
$unset: { ...(scriptCompiled ? { scriptError: 1 as const } : {}), - ...(scriptError ? { scriptCompiled: 1 as const } : {}), },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.tsapps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
🧰 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.tsapps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
🧠 Learnings (1)
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (3)
apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts (2)
119-125: Error handling correctly prevents silent failures.Throwing on compilation failure ensures the integration is not persisted with an empty
scriptCompiledfield, which aligns with the PR objective to surface errors to the Admin UI.
133-144: Channel resolution correctly uses derivedchannelName.Using
channelNamefor all lookups prevents the previous issue where the loop variable mutation could affect subsequent iterations or lookups.apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)
166-174: Script state update logic is sound for success path.The conditional inclusion of
scriptCompiledand clearing ofscriptErrorvia$unsetcorrectly maintains consistent script state. Note that line 173 (scriptError) is dead code as mentioned above.
|
I have addressed all feedback from @coderabbitai and @cubic-dev-ai:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12481 @Kalpana-chavhan Thank you for addressing all the feedback! I've verified each fix: ✅ Dead code removal: Both files now throw immediately on compilation failure with no unreachable code paths setting ✅ Atomic operations: ✅ Method name correction: All error objects in ✅ Modern syntax: Both files use ✅ Loop variables: All The refactored code is clean, atomic, and properly surfaces compilation errors to the Admin UI. Excellent work! 🎉 🐰✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)
81-83: Unawaited promise causes validation errors to fail silently.The promise returned by
wrapExceptions(...).catch(...)is not awaited. Throwing inside the.catch()callback results in an unhandled rejection rather than an error that stops execution. This directly undermines the PR's goal of surfacing script errors to administrators.🔎 Proposed fix
- wrapExceptions(() => validateScriptEngine(scriptEngine)).catch((e) => { + await wrapExceptions(() => validateScriptEngine(scriptEngine)).catch((e) => { throw new Meteor.Error(e.message); });
🧹 Nitpick comments (1)
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (1)
172-175: Consider clearingscriptErrorwhen script is disabled or emptied.Currently,
scriptErroris only unset when compilation succeeds (scriptCompiledis truthy). If a user disables the script or clears it entirely, any previous error remains in the database. This isn't necessarily wrong—it preserves error context for debugging—but verify this is the intended behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.tsapps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
🧰 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/updateIncomingIntegration.tsapps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts
🔇 Additional comments (6)
apps/meteor/app/integrations/server/methods/incoming/addIncomingIntegration.ts (3)
70-70: LGTM: Error context corrected.The method field now correctly references
addIncomingIntegrationinstead of the incorrectupdateIncomingIntegration.
117-122: LGTM: Script compilation now fails fast.The immediate throw on compilation failure prevents saving broken integrations and surfaces errors to the Admin UI, which directly addresses the PR objectives. The error message is descriptive and handles both Error instances and unknown error types safely.
125-141: LGTM: Cleaner channel processing with immutability.The refactor improves code clarity by:
- Using
constto prevent accidental mutation of the loop variable- Deriving
channelNameexplicitly viachannel.slice(1)- Using the derived
channelNameconsistently for all database lookupsThe lookup logic correctly handles both ID-based and name-based channel references (e.g.,
#roomIdvs#roomName,@userIdvs@username).apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts (3)
86-100: Good implementation of fail-fast compilation with proper error surfacing.The try/catch properly captures Babel compilation errors and throws a
Meteor.Errorthat will propagate to the Admin UI. Exiting before the database update ensures no broken state is persisted. This correctly addresses the original issue.
102-115: Clean channel handling without mutation.Using
const channeland derivingchannelNameseparately improves clarity and avoids the mutation pattern. The lookups correctly usechannelNamefor room/user identification.
147-178: Atomic update structure looks good.The single
findOneAndUpdatewith conditional field inclusion addresses previous concerns about partial database updates. Script state is now updated atomically with other integration fields.
|
Hi Maintainers, |
1 participant
Proposed changes
This PR fixes issue #37800 where incoming webhooks failed to process scripts using modern JS syntax (like nullish coalescing
??).@babel/plugin-syntax-nullish-coalescing-operatorto the server bundle.addIncomingIntegrationandupdateIncomingIntegrationto throw aMeteor.Errorupon compilation failure. This prevents silent failures and notifies the admin via the UI.Checklist
??now compile and execute.Steps to test
const x = request.content ?? {};.closes #37800
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.