feat: add onboarding chatbot flow for employee setup #37758
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 |
|
|
NullPointer-cell seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughA new Node.js Express webhook at POST /webhook implements a MongoDB-backed, stateful onboarding chatbot that persists per-user progress and responses and walks users through collecting department, employee ID, and location, with a global "help" handler. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Express /webhook
participant DB as MongoDB (Mongoose)
Client->>Server: POST /webhook { username, message }
Server->>DB: findOne({ username })
alt user exists
DB-->>Server: user doc (with step)
else new user
Server->>DB: create user { username, step:0 }
DB-->>Server: new user doc
end
Server->>Server: evaluate message & current step
alt "help" in message
Server-->>Client: FAQ/help response
else step 0: start command
Server->>DB: update step -> 1
DB-->>Server: updated doc
Server-->>Client: prompt for department
else step 1: record department
Server->>DB: update department, step -> 2
DB-->>Server: updated doc
Server-->>Client: prompt for Employee ID
else step 2: record employee ID
Server->>DB: update employeeId, step -> 3
DB-->>Server: updated doc
Server-->>Client: prompt for location
else step 3: record location
Server->>DB: update location, step -> 4
DB-->>Server: updated doc
Server-->>Client: send summary (department, ID, location)
else step 4: already completed
Server-->>Client: "onboarding completed" message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
onboarding-bot/index.js (3)
11-13: In-memory global state may not scale or survive restarts
stepsanddataare per-process globals, so state is lost on restart and inconsistent across multiple instances; they also grow unbounded with each new user.Consider moving this to a persistent store (e.g., DB/Redis) or at least adding a TTL/cleanup strategy if this bot is expected to run in production or behind multiple Node processes.
Please verify expected deployment topology (single vs multi-instance) before investing in persistence.
69-72: Consider allowing users to restart onboarding after completionOnce
steps[user] === 4, there is no way for a user to re-run onboarding by sending “onboarding” again, which might be useful for testing or if they answered incorrectly.One lightweight option:
- if (steps[user] === 4) { - return res.json({ text: "You have already completed onboarding! Type 'help' for other queries." }); - } + if (steps[user] === 4) { + if (msg === 'onboarding') { + steps[user] = 1; + data[user] = {}; + return res.json({ + text: "Restarting onboarding.\n\nFirst question: **Which department are you joining?**", + }); + } + return res.json({ + text: "You have already completed onboarding! Type 'onboarding' to restart or 'help' for other queries.", + }); + }This keeps the current behavior but adds an explicit restart path.
Please check with product/UX whether re-running onboarding is desired behavior before applying.
6-7: Inline comments are heavier than needed for production codeThere are many narrative comments (“WORK STARTS HERE”, step headings, explanatory bullets). As per the coding guidelines you shared, implementation code should avoid inline comments when the code is already self‑explanatory.
Once the flow stabilizes, consider pruning these to:
- Keep the handler lean and easier to scan.
- Rely on function/variable naming and, if needed, higher-level docs instead of in-line prose.
No functional change required; this is a style/maintainability cleanup.
Please double-check your project’s linting/CI rules; if they enforce comment style, aligning this file will prevent future noise.
Also applies to: 9-10, 14-15, 27-28, 38-39, 45-46, 52-53, 57-58, 69-71, 77-79
📜 Review details
Configuration used: CodeRabbit 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 (1)
onboarding-bot/index.js(1 hunks)
🧰 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:
onboarding-bot/index.js
🔇 Additional comments (1)
onboarding-bot/index.js (1)
16-18: Payload parsing hardening and case preservation for stored dataThe review identifies two concerns:
- Missing validation for
user_namefield, causing undefined values to group as"undefined"- Storing department/employeeId/location in lowercase, losing original user formatting
The suggested approach separates raw text (for storage) from normalized text (for comparisons), with early validation returning HTTP 400 on missing
user_name.However, verification of HTTP 400 response compatibility with Rocket.Chat webhook retry behavior and confirmation that lowercase storage is problematic in this context cannot be completed due to repository access limitations.
onboarding-bot/index.js
Outdated
| const summary = ` | ||
| 🎉 **Onboarding Complete!** | ||
| - Department: ${data[user].department} | ||
| - ID: ${data[user].employeeId} | ||
| - Location: ${data[user].location} | ||
|
|
||
| You can now type **'help'** if you have questions. | ||
| `; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "onboarding-bot" -type dRepository: RocketChat/Rocket.Chat
Length of output: 188
🌐 Web query:
Rocket.Chat message formatting custom parser markdown indentation
💡 Result:
Short answer
- Rocket.Chat uses its own message parser (@rocket.chat/message-parser) to parse markdown-like syntax (lists, quotes, code blocks, etc.). To change how indentation or other markdown is handled you must either (a) change/preprocess the text before posting, (b) modify/extend the message-parser grammar (or replace it) in the server code, or (c) avoid parser rules by using explicit code blocks or the Blocks/UIKit message types. [1][2][3]
Details / practical options
- Engine: the parser package is @rocket.chat/message-parser (parses quotes, ordered/unordered/task lists, blockquotes, code blocks, mentions, emojis, timestamps, etc.). Edit that package or fork it to change indentation rules. [1]
- Admin settings: older Rocket.Chat releases exposed selectable markdown settings (Marked, legacy options), but many of those parser-selection settings were removed in recent major updates—so swapping parsers via admin UI is no longer available in current versions; you’ll need to change server code/config or preprocess messages. [2]
- Workarounds without changing parser:
- Use fenced code blocks (``` ) to preserve exact indentation/whitespace.
- Pre-escape or transform leading characters (e.g., prefix lines with zero-width space or use backslash escapes) before posting.
- Use Blocks (UIKit) to send structured/plain_text content where you control rendering. [3][4]
If you want, tell me:
- which Rocket.Chat version you’re running and an example message (showing current vs desired rendering), and I’ll give a concrete code snippet or patch suggestion (preprocessing or message-parser change).
🏁 Script executed:
# Search for message formatting patterns in the codebase
rg -t js -t ts "template.*literal|join.*\\n" --max-count 20 -A 2 -B 2 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 188
Verify message rendering in Rocket.Chat test room
The indentation in this template literal may or may not affect rendering depending on Rocket.Chat's custom message parser behavior. Consider refactoring to an array with .join('\n') for explicit control over line breaks and consistency:
- const summary = `
- 🎉 **Onboarding Complete!**
- - Department: ${data[user].department}
- - ID: ${data[user].employeeId}
- - Location: ${data[user].location}
-
- You can now type **'help'** if you have questions.
- `;
+ const summary = [
+ '🎉 **Onboarding Complete!**',
+ `- Department: ${data[user].department}`,
+ `- ID: ${data[user].employeeId}`,
+ `- Location: ${data[user].location}`,
+ '',
+ "You can now type **'help'** if you have questions.",
+ ].join('\n');Test the current rendering to confirm whether bullets display as intended.
📝 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.
| const summary = ` | |
| 🎉 **Onboarding Complete!** | |
| - Department: ${data[user].department} | |
| - ID: ${data[user].employeeId} | |
| - Location: ${data[user].location} | |
| You can now type **'help'** if you have questions. | |
| `; | |
| const summary = [ | |
| '🎉 **Onboarding Complete!**', | |
| `- Department: ${data[user].department}`, | |
| `- ID: ${data[user].employeeId}`, | |
| `- Location: ${data[user].location}`, | |
| '', | |
| "You can now type **'help'** if you have questions.", | |
| ].join('\n'); |
🤖 Prompt for AI Agents
In onboarding-bot/index.js around lines 58 to 65, the template literal contains
leading indentation that could alter Rocket.Chat rendering; replace the
multi-line template string with an array of lines joined by '\n' (e.g., build an
array with the title, bullets using data[user].department,
data[user].employeeId, data[user].location, and the help line, then call
.join('\n')) to ensure explicit, consistent line breaks and no leading spaces,
and then test in the Rocket.Chat test room to confirm bullets render correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
onboarding-bot/package.json (1)
1-22: Confirm dependency versions and align with Rocket.Chat toolchainThe standalone package file looks fine, but please double‑check that:
dotenv,express, andmongooseversions are compatible with the Node version used in this repo’s CI/runtime.- These versions don’t conflict with (or silently diverge from) the main repo’s tooling expectations.
If the onboarding bot is meant to be an example rather than production code, consider noting that in the README/PR description so future maintainers understand lifecycle and support expectations.
onboarding-bot/index.js (2)
21-30: StrengthenUserschema with uniqueness and indexing onusernameRight now multiple documents can be created for the same
username, and lookups will be collection scans without an index. That’s fragile under concurrent requests and doesn’t scale.Consider enforcing uniqueness and adding an index:
-const UserSchema = new mongoose.Schema({ - username: String, - step: { type: Number, default: 0 }, // Tracks progress (0, 1, 2, 3...) - department: String, - employeeId: String, - location: String -}); +const UserSchema = new mongoose.Schema({ + username: { type: String, required: true, unique: true, index: true }, + step: { type: Number, default: 0 }, // Tracks progress (0, 1, 2, 3...) + department: String, + employeeId: String, + location: String, +});This reduces the chance of duplicate user rows and makes
findOne({ username })efficient.
8-27: Inline comments conflict with repo JS/TS style guidelineThis file has several inline/bucket comments (
// --- 1. CONNECT TO MONGODB ---,// STEP 0: Waiting to start, etc.). The Rocket.Chat guidelines for*.js/*.ts(x)prefer concise code without implementation comments.Consider removing or trimming these, relying on clear naming and function structure instead.
📜 Review details
Configuration used: CodeRabbit 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 ignored due to path filters (1)
onboarding-bot/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
onboarding-bot/index.js(1 hunks)onboarding-bot/package.json(1 hunks)
🧰 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:
onboarding-bot/index.js
🧠 Learnings (4)
📓 Common learnings
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.
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
onboarding-bot/index.js
📚 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:
onboarding-bot/index.js
📚 Learning: 2025-11-19T12:32:29.696Z
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.
Applied to files:
onboarding-bot/index.js
🔇 Additional comments (1)
onboarding-bot/index.js (1)
33-95: Harden/webhookhandler: error handling, PII logging, and text normalizationThe flow is clear, but several changes would make it safer and more robust:
Async error handling
Any rejected Mongoose call (findOne,save) will currently bubble out of theasynchandler and can crash the process (Express 4 doesn't automatically catch rejected promises). Wrap the body in atry/catchand return a 500 on failures.Logging PII in
msg
At steps 1–3,msgcontainsdepartment,employeeId, andlocation, soconsole.log(... Msg: ${msg})is logging sensitive data. That's a compliance/privacy risk; better to avoid logging raw free‑form user input here.Lowercasing stored data
msgis lowercased upfront and then reused for storage, so you'll end up withdepartment,employeeId, andlocationall in lowercase, which is likely undesirable (IDs can be case‑sensitive, and locations look odd).Webhook authentication
As written, any caller can POST to/webhookand populate the onboarding collection. In production you probably want to validate a shared secret/header/token from Rocket.Chat before processing.One possible refactor that addresses 1–3 (and adds a simple
user_nameguard) while preserving the flow:-app.post('/webhook', async (req, res) => { - const username = req.body.user_name; - const msg = req.body.text ? req.body.text.trim().toLowerCase() : ""; - - // Find the user in the DB, or create them if they don't exist - let user = await User.findOne({ username: username }); - - if (!user) { - user = new User({ username: username, step: 0 }); - await user.save(); - } - - console.log(`User: ${username} | Step: ${user.step} | Msg: ${msg}`); - - // --- FAQ LOGIC (Global) --- - if (msg.includes('help')) { - return res.json({ text: "🤖 I can help. Ask about 'documents' or 'HR'." }); - } - - // --- CONVERSATION FLOW --- - - // STEP 0: Waiting to start - if (user.step === 0) { - if (msg === 'onboarding') { - user.step = 1; - await user.save(); // Save progress to DB - return res.json({ text: "👋 Welcome! **Which department are you joining?**" }); - } - return res.json({ text: `Hi ${username}! Type **'onboarding'** to start.` }); - } - - // STEP 1: Department - if (user.step === 1) { - user.department = msg; - user.step = 2; - await user.save(); // Save to DB - return res.json({ text: `Got it. **What is your Employee ID?**` }); - } - - // STEP 2: Employee ID - if (user.step === 2) { - user.employeeId = msg; - user.step = 3; - await user.save(); // Save to DB - return res.json({ text: "Thanks. **Which city is your work location?**" }); - } - - // STEP 3: Location (Finish) - if (user.step === 3) { - user.location = msg; - user.step = 4; - await user.save(); // Save final state - - return res.json({ text: `🎉 **Done!**\nDept: ${user.department}\nID: ${user.employeeId}\nLoc: ${user.location}` }); - } - - // STEP 4: Already Done - if (user.step === 4) { - return res.json({ text: "You have already completed onboarding." }); - } - - return res.json({ text: "I didn't understand that." }); -}); +app.post('/webhook', async (req, res) => { + try { + const username = req.body.user_name; + const rawText = typeof req.body.text === 'string' ? req.body.text.trim() : ''; + const msg = rawText.toLowerCase(); + + if (!username) { + return res.status(400).json({ text: 'Missing user_name in payload.' }); + } + + // Find the user in the DB, or create them if they don't exist + let user = await User.findOne({ username }); + + if (!user) { + user = new User({ username, step: 0 }); + await user.save(); + } + + console.log(`User: ${username} | Step: ${user.step}`); + + // --- FAQ LOGIC (Global) --- + if (msg.includes('help')) { + return res.json({ text: "🤖 I can help. Ask about 'documents' or 'HR'." }); + } + + // --- CONVERSATION FLOW --- + + // STEP 0: Waiting to start + if (user.step === 0) { + if (msg === 'onboarding') { + user.step = 1; + await user.save(); // Save progress to DB + return res.json({ text: "👋 Welcome! **Which department are you joining?**" }); + } + return res.json({ text: `Hi ${username}! Type **'onboarding'** to start.` }); + } + + // STEP 1: Department + if (user.step === 1) { + user.department = rawText; + user.step = 2; + await user.save(); // Save to DB + return res.json({ text: `Got it. **What is your Employee ID?**` }); + } + + // STEP 2: Employee ID + if (user.step === 2) { + user.employeeId = rawText; + user.step = 3; + await user.save(); // Save to DB + return res.json({ text: "Thanks. **Which city is your work location?**" }); + } + + // STEP 3: Location (Finish) + if (user.step === 3) { + user.location = rawText; + user.step = 4; + await user.save(); // Save final state + + return res.json({ text: `🎉 **Done!**\nDept: ${user.department}\nID: ${user.employeeId}\nLoc: ${user.location}` }); + } + + // STEP 4: Already Done + if (user.step === 4) { + return res.json({ text: "You have already completed onboarding." }); + } + + return res.json({ text: "I didn't understand that." }); + } catch (err) { + console.error('Error handling /webhook', err); + return res.status(500).json({ text: 'Sorry, something went wrong. Please try again later.' }); + } +});You can then layer in a simple shared‑secret check (e.g., compare a header or token param against
process.env.WEBHOOK_SECRET) near the top of the handler if you intend to expose this endpoint beyond a trusted network.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
onboarding-bot/index.js (3)
20-29: Add index/uniqueness tousernamein the Mongoose schemaRight now
usernameis a plainString, but the flow always doesfindOne({ username }), and nothing prevents duplicate documents for the same user. That can cause non‑deterministic onboarding state and slower lookups at scale.Consider marking
usernameas indexed and unique:-const UserSchema = new mongoose.Schema({ - username: String, - step: { type: Number, default: 0 }, - department: String, - employeeId: String, - location: String -}); +const UserSchema = new mongoose.Schema({ + username: { type: String, required: true, unique: true, index: true }, + step: { type: Number, default: 0 }, + department: String, + employeeId: String, + location: String, +});
36-36: Remove inline implementation comments to match repo guidelinesThere are inline comments (
// Find the user...,// Save final step,// Start Server) in implementation code. The repository guidelines for*.jsfiles say to avoid code comments in the implementation.These lines can be safely removed; the code is self‑explanatory:
- // Find the user in the DB, or create them if they don't exist @@ - await user.save(); // Save final step + await user.save(); @@ -// Start ServerAlso applies to: 80-80, 93-93
11-16: Optionally gate server startup on successful MongoDB connection
app.listenis called regardless of whethermongoose.connectsucceeds. If MongoDB is unavailable, the bot will still accept HTTP traffic but every request will fail at runtime.If this is meant to be DB‑backed only, consider starting the server after a successful connection:
-mongoose.connect(MONGODB_URI) - .then(() => console.log('✅ Connected to MongoDB')) - .catch(err => { - console.error(' MongoDB Connection Error:', err.message); - console.log(' Make sure MongoDB is running or set MONGODB_URI environment variable'); - }); - -const PORT = process.env.PORT || 3000; -app.listen(PORT, () => { - console.log(`Bot running on port ${PORT}`); -}); +mongoose.connect(MONGODB_URI) + .then(() => { + console.log('✅ Connected to MongoDB'); + const PORT = process.env.PORT || 3000; + app.listen(PORT, () => { + console.log(`Bot running on port ${PORT}`); + }); + }) + .catch(err => { + console.error(' MongoDB Connection Error:', err.message); + console.log(' Make sure MongoDB is running or set MONGODB_URI environment variable'); + process.exit(1); + });Also applies to: 93-97
📜 Review details
Configuration used: CodeRabbit 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 (1)
onboarding-bot/index.js(1 hunks)
🧰 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:
onboarding-bot/index.js
🧠 Learnings (4)
📓 Common learnings
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.
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
onboarding-bot/index.js
📚 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:
onboarding-bot/index.js
📚 Learning: 2025-11-19T12:32:29.696Z
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.
Applied to files:
onboarding-bot/index.js
| app.post('/webhook', async (req, res) => { | ||
| const username = req.body.user_name; |
There was a problem hiding this comment.
Consider basic authentication/token checking for the webhook
/webhook currently accepts any POST with user_name and text and exposes onboarding data over HTTP with no authentication or token validation. For a production Rocket.Chat integration, this endpoint should at least verify a shared secret (e.g., query parameter, header, or request body token) or be restricted behind network controls.
A minimal pattern would be:
-const PORT = process.env.PORT || 3000;
+const PORT = process.env.PORT || 3000;
+const WEBHOOK_TOKEN = process.env.WEBHOOK_TOKEN;
@@
-app.post('/webhook', async (req, res, next) => {
- try {
+app.post('/webhook', async (req, res, next) => {
+ try {
+ if (WEBHOOK_TOKEN && req.body.token !== WEBHOOK_TOKEN) {
+ return res.status(403).json({ text: 'Forbidden.' });
+ }Also applies to: 93-97
| app.post('/webhook', async (req, res) => { | ||
| const username = req.body.user_name; | ||
| const msg = req.body.text ? req.body.text.trim().toLowerCase() : ""; | ||
|
|
||
| // Find the user in the DB, or create them if they don't exist | ||
| let user = await User.findOne({ username: username }); | ||
|
|
||
| if (!user) { | ||
| user = new User({ username: username, step: 0 }); | ||
| await user.save(); | ||
| } |
There was a problem hiding this comment.
Validate user_name and avoid creating a shared undefined user
If req.body.user_name is missing or falsy, findOne({ username }) will search for username: undefined, and the if (!user) block will persist a record with username: undefined. All such bad requests will then share the same onboarding state and data, which is both confusing and a privacy risk.
Add basic validation and return a 4xx instead of proceeding:
app.post('/webhook', async (req, res) => {
- const username = req.body.user_name;
+ const username = req.body.user_name;
+ if (!username || typeof username !== 'string') {
+ return res.status(400).json({ text: 'Missing or invalid user name.' });
+ }📝 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.
| app.post('/webhook', async (req, res) => { | |
| const username = req.body.user_name; | |
| const msg = req.body.text ? req.body.text.trim().toLowerCase() : ""; | |
| // Find the user in the DB, or create them if they don't exist | |
| let user = await User.findOne({ username: username }); | |
| if (!user) { | |
| user = new User({ username: username, step: 0 }); | |
| await user.save(); | |
| } | |
| app.post('/webhook', async (req, res) => { | |
| const username = req.body.user_name; | |
| if (!username || typeof username !== 'string') { | |
| return res.status(400).json({ text: 'Missing or invalid user name.' }); | |
| } | |
| const msg = req.body.text ? req.body.text.trim().toLowerCase() : ""; | |
| // Find the user in the DB, or create them if they don't exist | |
| let user = await User.findOne({ username: username }); | |
| if (!user) { | |
| user = new User({ username: username, step: 0 }); | |
| await user.save(); | |
| } |
🤖 Prompt for AI Agents
in onboarding-bot/index.js around lines 32 to 42, the handler uses
req.body.user_name without validation which causes a DB lookup and possible
creation of a user record with username undefined; change the code to first
validate that req.body.user_name exists and is a non-empty string (trim it), and
if it is missing/empty respond with a 400 (and optional error message) without
querying or creating a User; only proceed to call User.findOne / new User when a
valid username is present (and consider logging the invalid request for
monitoring).
| app.post('/webhook', async (req, res) => { | ||
| const username = req.body.user_name; | ||
| const msg = req.body.text ? req.body.text.trim().toLowerCase() : ""; | ||
|
|
||
| // Find the user in the DB, or create them if they don't exist | ||
| let user = await User.findOne({ username: username }); | ||
|
|
||
| if (!user) { | ||
| user = new User({ username: username, step: 0 }); | ||
| await user.save(); | ||
| } | ||
|
|
||
| console.log(`User: ${username} | Step: ${user.step} | Msg: ${msg}`); | ||
|
|
||
|
|
||
| if (msg.includes('help')) { | ||
| return res.json({ text: "🤖 I can help. Ask about 'documents' or 'HR'." }); | ||
| } | ||
|
|
||
| if (user.step === 0) { | ||
| if (msg === 'onboarding') { | ||
| user.step = 1; | ||
| await user.save(); | ||
| return res.json({ text: "👋 Welcome! **Which department are you joining?**" }); | ||
| } | ||
| return res.json({ text: `Hi ${username}! Type **'onboarding'** to start.` }); | ||
| } | ||
|
|
||
|
|
||
| if (user.step === 1) { | ||
| user.department = msg; | ||
| user.step = 2; | ||
| await user.save(); | ||
| return res.json({ text: `Got it. **What is your Employee ID?**` }); | ||
| } | ||
|
|
||
|
|
||
| if (user.step === 2) { | ||
| user.employeeId = msg; | ||
| user.step = 3; | ||
| await user.save(); | ||
| return res.json({ text: "Thanks. **Which city is your work location?**" }); | ||
| } | ||
|
|
||
|
|
||
| if (user.step === 3) { | ||
| user.location = msg; | ||
| user.step = 4; | ||
| await user.save(); // Save final step | ||
|
|
||
| return res.json({ text: ` **Done!**\nDept: ${user.department}\nID: ${user.employeeId}\nLoc: ${user.location}` }); | ||
| } | ||
|
|
||
|
|
||
| if (user.step === 4) { | ||
| return res.json({ text: "You have already completed onboarding." }); | ||
| } | ||
|
|
||
| return res.json({ text: "I didn't understand that." }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n onboarding-bot/index.jsRepository: RocketChat/Rocket.Chat
Length of output: 188
🏁 Script executed:
# Search for error handling middleware in the codebase
rg -i "app\.(use|error)" --type jsRepository: RocketChat/Rocket.Chat
Length of output: 188
🏁 Script executed:
# Check if there are any try-catch patterns elsewhere in the codebase
rg "try\s*\{" --type js -A 3Repository: RocketChat/Rocket.Chat
Length of output: 188
🏁 Script executed:
# Verify the middleware setup and app configuration
rg "express\(\)|app\s*=" --type js -B 2 -A 5Repository: RocketChat/Rocket.Chat
Length of output: 188
🌐 Web query:
Express.js 4 async route handler unhandled promise rejection error handling
💡 Result:
Short answer: Express 4 does not automatically catch rejected Promises from async route handlers — you must forward rejections to Express (next(err)) or use a wrapper/patch so rejected Promises get passed to your error handler. See Express docs and community solutions below. [1][2]
Recommended fixes (concise)
- Short-term, explicit try/catch and next(err)
- Put try/catch inside async handlers and call next(err) on failure.
Example:
const handler = async (req,res,next) => {
try {
const data = await doAsync();
res.json(data);
} catch (err) {
next(err);
}
};
- Use a small async wrapper to auto-forward rejections
-
Create once and wrap every async route:
const asyncHandler = fn => (req,res,next) => Promise.resolve(fn(req,res,next)).catch(next);
app.get('/x', asyncHandler(async (req,res) => { ... })); -
Or use a maintained package: express-async-handler or express-async-errors (require once) to avoid repetitive try/catch. [2][3]
- Ensure a final error-handling middleware is mounted
- Add (err, req, res, next) => { log; if (!res.headersSent) res.status(500).json({error:err.message}); } after all routes. [1]
- Optional safety: log unhandled rejections
- Add process.on('unhandledRejection', (reason, p) => { /* log/alert */ }); while you fix route handling. This helps diagnose silent failures. [4]
Why this is necessary
- Express 4 doesn’t inspect the Promise returned by an async function, so a rejected Promise becomes an unhandled rejection unless you call next(err) or wrap the handler. Express 5 changes this behavior and will forward Promise rejections automatically. [1][2]
Sources
- Express error-handling guide. [1]
- Community discussion and async-wrapper pattern (asyncHandler). [2]
- express-async-errors / express-async-handler packages (npm). [3]
- Notes on logging unhandled rejections. [4]
🌐 Web query:
Express.js async await try catch error handling best practices
💡 Result:
Here are concise best practices for handling async/await errors in Express (with short patterns):
-
Always forward async errors to Express error middleware (use next). Example:
- async (req, res, next) => { try { ... } catch (err) { next(err) } }. [1][3]
-
Use a small wrapper to avoid repeating try/catch (catchAsync):
- const catchAsync = fn => (req,res,next) => fn(req,res,next).catch(next); then app.get('/x', catchAsync(async (req,res)=>{...})). [2][4]
-
Consider a vetted patch/package that auto-forwards thrown async errors (e.g., express-async-errors) — import it early if you use it. Vet maintenance/security before adopting. [2][5]
-
Centralize handling in one error-handling middleware (last middleware signature (err, req, res, next)): map errors → status codes, sanitize messages for production, and log details separately. [1][4]
-
Use custom Error classes (statusCode, type) so handlers can respond consistently (400/401/404/500). Throw app errors from controllers. [4]
-
Don’t leak internals in production: return generic messages to clients, full stack only in dev logs. Use structured logging (Winston/Pino) and avoid logging secrets. [4][6]
-
Validate inputs early (middleware) to reduce runtime errors and return 4xx for client faults. [4][6]
-
Handle global process-level failures: listen for unhandledRejection and uncaughtException to log/cleanup and restart safely (use a process manager). [6]
-
Test error flows and monitor in production (APM, error reporting) so you catch unexpected rejections. [4][6]
Sources
- Express error-handling guidelines and patterns. [1]
- catchAsync / try-catch wrapper pattern and examples. [2]
- Why Express (v4) won’t automatically catch rejected async promises (explanation). [3]
- Best-practice patterns: custom errors, centralized middleware, sanitize messages, logging, validation. [4][6]
References
[1] Express error handling guide and async patterns.
[2] catchAsync wrapper / examples (how-to guides).
[3] Explanation of async function promise rejections vs. sync throws (StackOverflow).
[4] Best-practice writeups: custom errors, central middleware, env differentiation.
[6] Security/operational advice: logging, input validation, process-level handlers.
If you want, I can paste minimal copy-paste-ready code for a complete example (catchAsync + custom Error class + centralized handler + process handlers).
Wrap async route body in error handling for Express 4
The handler is async with several await calls; if MongoDB is down or an operation rejects, Express 4 will not automatically catch the promise rejection, which can result in unhandled rejections or process crashes instead of a clean 5xx response.
Wrap the logic in try/catch and forward to error middleware or respond directly:
-app.post('/webhook', async (req, res) => {
- const username = req.body.user_name;
+app.post('/webhook', async (req, res, next) => {
+ try {
+ const username = req.body.user_name;
@@
- return res.json({ text: "I didn't understand that." });
-});
+ return res.json({ text: "I didn't understand that." });
+ } catch (err) {
+ console.error('Webhook error:', err);
+ return next(err);
+ }
+});Alternatively, use a wrapper function or the express-async-errors package to avoid repetitive try/catch blocks across multiple async routes.
🤖 Prompt for AI Agents
In onboarding-bot/index.js around lines 32 to 91, the async POST /webhook
handler contains multiple awaits but lacks error handling; wrap the entire route
body in a try/catch (or replace the handler with an async wrapper) and on catch
call next(err) to forward to Express error middleware (or send
res.status(500).json({ error: 'Internal Server Error' }) if you prefer direct
handling); ensure any partial updates are handled appropriately and keep
existing response flow inside the try block, or alternatively install/use
express-async-errors or a shared asyncHandler to avoid repeating try/catch
across routes.
| const username = req.body.user_name; | ||
| const msg = req.body.text ? req.body.text.trim().toLowerCase() : ""; | ||
|
|
There was a problem hiding this comment.
Separate normalized command text from stored user data
msg is lowercased and then reused for department, employeeId, and location, which will silently lose formatting and case (e.g., “HR & Payroll” → “hr & payroll”, “NYC-01” → “nyc-01”).
Keep a trimmed raw string for storage and a normalized version for command logic:
- const msg = req.body.text ? req.body.text.trim().toLowerCase() : "";
+ const rawText = typeof req.body.text === 'string' ? req.body.text.trim() : '';
+ const msg = rawText.toLowerCase();
@@
- if (user.step === 1) {
- user.department = msg;
+ if (user.step === 1) {
+ user.department = rawText;
@@
- if (user.step === 2) {
- user.employeeId = msg;
+ if (user.step === 2) {
+ user.employeeId = rawText;
@@
- if (user.step === 3) {
- user.location = msg;
+ if (user.step === 3) {
+ user.location = rawText;Also applies to: 61-80
🤖 Prompt for AI Agents
In onboarding-bot/index.js around lines 33-35 (and similarly lines 61-80), the
code currently lowercases and trims req.body.text into msg and then reuses that
same value when storing user fields, causing stored values like department,
employeeId, and location to lose original casing/formatting; fix this by keeping
a separate trimmedRaw variable (trimmed but not lowercased) for persisting user
data and use a distinct normalized variable (trimmed + toLowerCase()) only for
command parsing/logic, and update any downstream assignments and validations to
use trimmedRaw for storage and normalized for comparisons.
| if (msg.includes('help')) { | ||
| return res.json({ text: "🤖 I can help. Ask about 'documents' or 'HR'." }); | ||
| } |
There was a problem hiding this comment.
Use an exact match for the help command instead of includes
msg.includes('help') will treat any input containing “help” (e.g., “helpdesk”, “my department is helpdesk”) as a help command, which breaks the onboarding flow at step 1.
Prefer an exact command check (after trimming/lowercasing), or a small set of aliases:
- if (msg.includes('help')) {
+ if (msg === 'help') {
return res.json({ text: "🤖 I can help. Ask about 'documents' or 'HR'." });
}📝 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.
| if (msg.includes('help')) { | |
| return res.json({ text: "🤖 I can help. Ask about 'documents' or 'HR'." }); | |
| } | |
| if (msg === 'help') { | |
| return res.json({ text: "🤖 I can help. Ask about 'documents' or 'HR'." }); | |
| } |
🤖 Prompt for AI Agents
In onboarding-bot/index.js around lines 47 to 49, the code uses
msg.includes('help') which matches substrings like "helpdesk" and breaks the
flow; change this to compare a normalized message (trim() and toLowerCase())
against an exact string or a small alias set (e.g., const m =
msg.trim().toLowerCase(); if (m === 'help' || ['help','h','?'].includes(m)) ...)
or use a regex with anchors (/^help$/i) so only an exact "help" command triggers
the help response.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.