fix: Handle string boolean parameter in loadSurroundingMessages (#40119)#40193
fix: Handle string boolean parameter in loadSurroundingMessages (#40119)#40193manaswi-siripurapu wants to merge 2 commits intoRocketChat:developfrom
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 |
|
|
|
WalkthroughThe change adds string-to-boolean coercion for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ 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. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/server/methods/loadSurroundingMessages.ts (1)
14-17: Consider widening the declaredshowThreadMessagestype to reflect accepted inputs.The interface still advertises
showThreadMessages?: boolean, yet the method now intentionally accepts string inputs from mobile/REST clients. If you intend to keep coercing strings, widen the DDP-client typing (e.g.showThreadMessages?: boolean | string) so typed callers aren't misled and the server contract matches the runtime behavior. Otherwise, if the canonical contract is truly boolean-only, the coercion is patching around a client bug — in that case a clearer server-side error (or fixing the offending client) may be preferable to silent coercion of arbitrary strings tofalse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/methods/loadSurroundingMessages.ts` around lines 14 - 17, The declared parameter type for showThreadMessages on loadSurroundingMessages is too narrow (boolean) while the runtime accepts string inputs; update the method signature to widen the type to showThreadMessages?: boolean | string (or another union that reflects accepted inputs) so TypeScript callers match runtime behavior, and ensure any existing internal coercion still handles string values (e.g., in loadSurroundingMessages) or replace the coercion with a clear validation error if you prefer enforcing boolean-only inputs.
🤖 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/server/methods/loadSurroundingMessages.ts`:
- Around line 32-37: Remove the inline comment and extract the string-to-boolean
coercion into a local variable (e.g. normalizedShowThreadMessages): detect if
showThreadMessages is a string, normalize/trim/lowercase it and set
normalizedShowThreadMessages to the boolean result, then use
normalizedShowThreadMessages in place of showThreadMessages for the subsequent
check(showThreadMessages, Boolean) and wherever showThreadMessages is referenced
later (e.g. the usages formerly at lines 73 and 90) to improve type clarity and
readability; leave the original showThreadMessages parameter unchanged.
---
Nitpick comments:
In `@apps/meteor/server/methods/loadSurroundingMessages.ts`:
- Around line 14-17: The declared parameter type for showThreadMessages on
loadSurroundingMessages is too narrow (boolean) while the runtime accepts string
inputs; update the method signature to widen the type to showThreadMessages?:
boolean | string (or another union that reflects accepted inputs) so TypeScript
callers match runtime behavior, and ensure any existing internal coercion still
handles string values (e.g., in loadSurroundingMessages) or replace the coercion
with a clear validation error if you prefer enforcing boolean-only inputs.
🪄 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: c0a8b54f-68da-4fda-8b41-89cb7ecab696
📒 Files selected for processing (1)
apps/meteor/server/methods/loadSurroundingMessages.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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/server/methods/loadSurroundingMessages.ts
🧠 Learnings (4)
📚 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/server/methods/loadSurroundingMessages.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/server/methods/loadSurroundingMessages.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/server/methods/loadSurroundingMessages.ts
📚 Learning: 2026-03-09T18:39:14.020Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:14.020Z
Learning: When implementing batch processing in server methods, favor a single data pass to collect both the items and any derived fields needed for validation. Use the same dataset for both validation and processing to avoid races between validation and execution, and document the approach in code comments. Apply this pattern to similar Meteor Rocket.Chat server methods under apps/meteor/server/methods to prevent race conditions and ensure consistent batch behavior.
Applied to files:
apps/meteor/server/methods/loadSurroundingMessages.ts
| // Coerce string to boolean (common when parameters come from mobile/REST clients) | ||
| if (typeof showThreadMessages === 'string') { | ||
| showThreadMessages = showThreadMessages.trim().toLowerCase(); | ||
| showThreadMessages = showThreadMessages === 'true' || showThreadMessages === '1'; | ||
| } | ||
| check(showThreadMessages, Boolean); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm strict/noImplicitAny settings that would surface the narrowing-to-never issue.
fd -t f 'tsconfig*.json' apps/meteor | head -20 | xargs -I {} sh -c 'echo "=== {} ==="; cat {}'Repository: RocketChat/Rocket.Chat
Length of output: 1448
🏁 Script executed:
cat -n apps/meteor/server/methods/loadSurroundingMessages.ts | head -100Repository: RocketChat/Rocket.Chat
Length of output: 3381
🏁 Script executed:
rg "typeof.*===.*'string'" apps/meteor/server/methods --type ts -A 3 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 1045
🏁 Script executed:
rg "showThreadMessages" apps/meteor --type ts -B 2 -A 2 | head -60Repository: RocketChat/Rocket.Chat
Length of output: 3852
🏁 Script executed:
cat -n apps/meteor/server/methods/loadHistory.ts | head -80Repository: RocketChat/Rocket.Chat
Length of output: 2699
🏁 Script executed:
rg -A 30 "export.*loadMessageHistory\|function loadMessageHistory" apps/meteor/app/lib/server/functions/loadMessageHistory.ts | head -80Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
find apps/meteor -name "*loadMessageHistory*" -type fRepository: RocketChat/Rocket.Chat
Length of output: 124
🏁 Script executed:
cat -n apps/meteor/app/lib/server/functions/loadMessageHistory.ts | head -150Repository: RocketChat/Rocket.Chat
Length of output: 3071
🏁 Script executed:
rg "findVisibleByRoomIdBeforeTimestamp|findVisibleByRoomIdAfterTimestamp" apps/meteor --type ts -B 2 -A 8 | grep -A 10 "showThreadMessages"Repository: RocketChat/Rocket.Chat
Length of output: 2923
🏁 Script executed:
git log --oneline apps/meteor/server/methods/loadSurroundingMessages.ts | head -10Repository: RocketChat/Rocket.Chat
Length of output: 137
🏁 Script executed:
git show a0f18bc623 --statRepository: RocketChat/Rocket.Chat
Length of output: 50379
🏁 Script executed:
git show a0f18bc623 -- apps/meteor/server/methods/loadSurroundingMessages.tsRepository: RocketChat/Rocket.Chat
Length of output: 3283
🏁 Script executed:
cd apps/meteor && npx tsc --noEmit --skipLibCheck server/methods/loadSurroundingMessages.ts 2>&1 | head -30Repository: RocketChat/Rocket.Chat
Length of output: 207
🏁 Script executed:
cd apps/meteor && npx tsc --noEmit 2>&1 | grep -A 5 "loadSurroundingMessages"Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
rg "showThreadMessages" apps/meteor/server/methods --type ts -B 5 -A 10 | grep -A 15 "loadSurroundingMessages(message"Repository: RocketChat/Rocket.Chat
Length of output: 1529
Remove inline comment; consider using a local variable for type clarity.
The inline comment on line 32 violates coding guidelines ("Avoid code comments in the implementation"). While the string-to-boolean coercion pattern is intentional and handles REST/mobile client inputs that send strings instead of booleans, extracting the normalization into a local variable would improve type safety and readability:
♻️ Optional refactor
- async loadSurroundingMessages(message, limit = 50, showThreadMessages = true) {
+ async loadSurroundingMessages(message, limit = 50, showThreadMessages: unknown = true) {
check(message, Object);
check(limit, Number);
- // Coerce string to boolean (common when parameters come from mobile/REST clients)
- if (typeof showThreadMessages === 'string') {
- showThreadMessages = showThreadMessages.trim().toLowerCase();
- showThreadMessages = showThreadMessages === 'true' || showThreadMessages === '1';
- }
- check(showThreadMessages, Boolean);
+ const normalizedShowThreadMessages =
+ typeof showThreadMessages === 'string'
+ ? ['true', '1'].includes(showThreadMessages.trim().toLowerCase())
+ : showThreadMessages;
+ check(normalizedShowThreadMessages, Boolean);Then reference normalizedShowThreadMessages on lines 73 and 90.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/server/methods/loadSurroundingMessages.ts` around lines 32 - 37,
Remove the inline comment and extract the string-to-boolean coercion into a
local variable (e.g. normalizedShowThreadMessages): detect if showThreadMessages
is a string, normalize/trim/lowercase it and set normalizedShowThreadMessages to
the boolean result, then use normalizedShowThreadMessages in place of
showThreadMessages for the subsequent check(showThreadMessages, Boolean) and
wherever showThreadMessages is referenced later (e.g. the usages formerly at
lines 73 and 90) to improve type clarity and readability; leave the original
showThreadMessages parameter unchanged.
3 participants
Proposed changes
Fixes #40119
Users on Android app v4.71.2 and Desktop app v4.14 experience crashes when loading surrounding messages due to
showThreadMessagesparameter being sent as a string instead of boolean.Problem
Error:
Match error: Expected boolean, got stringSolution
Added automatic coercion of string boolean values to proper booleans before validation:
trueand anything else asfalseTesting
Closes #40119
Summary by CodeRabbit