Open
Conversation
Contributor
WalkthroughThe pull request introduces a Redis-backed deduplication system for user command caching, implements role-based caching with fallback adapter, refactors the managed-commands system with improved permission checking and scope-based command filtering, updates command type signatures to remove redundant context parameters, and adds utility functions for async array operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ManagedCommands
participant Redis as Redis<br/>(cachedUserSetCommands)
participant RolesCache as Redis<br/>(userRolesCache)
participant API as api.tg.permissions
participant Telegram as Telegram Bot API
Client->>ManagedCommands: Message received (user in group)
ManagedCommands->>Redis: cachedUserSetCommands(userId, chatId)
alt Cache hit (returns true)
Redis-->>ManagedCommands: true
ManagedCommands->>Telegram: Skip command menu update
else Cache miss (returns false)
Redis-->>ManagedCommands: false
ManagedCommands->>RolesCache: get(userId)
alt Roles cached
RolesCache-->>ManagedCommands: roles[]
else Roles not cached
RolesCache->>API: getRoles(userId)
API-->>RolesCache: roles[]
RolesCache->>RolesCache: cache for 5 min
end
ManagedCommands->>ManagedCommands: Filter commands by scope & roles
ManagedCommands->>Telegram: setMyCommands(scope: chat_member, userId, chatId)
Telegram-->>ManagedCommands: ✓
ManagedCommands->>Redis: Mark as cached
end
ManagedCommands-->>Client: Command available
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/managed-commands/index.ts (1)
419-427:⚠️ Potential issue | 🟠 Major
/help <command>is bypassing the new permission filter.This branch searches
this.getCommands()directly and returns usage even for commands the caller could not execute or see in the normal/helplist. That leaks hidden/admin-only commands and breaks the "same permissions checks as elsewhere" goal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/managed-commands/index.ts` around lines 419 - 427, The /help <command> branch is querying this.getCommands() directly and can return usage for commands the caller shouldn't see; replace the direct this.getCommands() lookup with the same permission-filtered command list used by the normal help flow (i.e., apply the existing permission-check helper or the filtered-getter the codebase uses instead of this.getCommands()) before finding the command, then call ManagedCommands.formatCommandUsage(cmd) and ctx.reply as before; in short, perform the same permission filtering when resolving the specific command as is used elsewhere so hidden/admin-only commands are not returned.
🧹 Nitpick comments (1)
src/utils/arrays.ts (1)
7-9: Consider bounded concurrency for large inputs.
Promise.all(arr.map(...))launches all tasks at once. For large arrays, this can create avoidable memory and downstream API/DB pressure. Consider adding a concurrency-limited variant (or optionalconcurrencyparam) for safer reuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/arrays.ts` around lines 7 - 9, The current asyncMap function launches all promises at once causing potential memory/IO pressure; update asyncMap to support bounded concurrency by adding an optional concurrency parameter (e.g., asyncMap<T,U>(arr: T[], mapper: (item:T)=>Promise<U>, concurrency?: number)) or create a new asyncMapWithConcurrency helper that runs at most N mapper tasks concurrently; implement this by iterating the array and scheduling tasks through a simple semaphore/worker queue or using a tiny internal pool (driven by the provided concurrency value, defaulting to Infinity to preserve current behavior), ensure the returned Promise resolves to U[] in original order and reference the existing asyncMap symbol so callers can switch to the concurrency-aware variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/managed-commands/index.ts`:
- Around line 524-535: The group-only permission block is being applied for
commands with scope "both" even in private chats; update the logic in the
permission check so that when the current chat is private (check ctx.chat.type
or equivalent), you skip the isAllowedInGroups branch entirely — i.e., only run
isAllowedInGroups(command) && isGroupChat check before calling
this.isCommandAllowedInGroup(command, ctx.chat.id) and evaluating
allowGroupAdmins via isFromGroupAdmin(); otherwise, directly proceed to
getUserRoles() and this.isCommandAllowedForRoles(command, roles).
- Around line 403-410: The cache check using
this.hooks.cachedUserSetCommands?.(ctx.from.id, ctx.chat.id) is done before
calling ctx.api.setMyCommands and any API failure is swallowed, so you must only
mark the user:chat as cached after setMyCommands completes successfully: keep
the initial check to skip if already cached, but move the code that records/sets
the cache (the hook or setter you have for marking a user/chat as cached) into
the successful path of await
ctx.api.setMyCommands(allowedCommands.flatMap(toBotCommands), { scope: { type:
"chat_member", chat_id: ctx.chat.id, user_id: ctx.from.id } }) — do not set the
cache in the .catch handler and ensure the .catch still logs or surfaces errors
as needed so transient Telegram failures won’t incorrectly mark the pair as
cached; use getAllowedCommandsFor, toBotCommands and the hooks.* methods to
locate the exact lines to change.
In `@src/redis/set.ts`:
- Around line 59-65: flushMemoryCache() is calling .map() on
Set.prototype.values() (an iterator), causing TypeError; fix by converting the
iterators to arrays before mapping (e.g., use
Array.from(this.memoryCache.values()) or [...this.memoryCache]) and do the same
for this.deletions so Promise.all receives an array of promises; apply the
identical change to the mirrored methods in
src/lib/redis-fallback-adapter/index.ts (the calls at the corresponding lines).
- Around line 78-82: The _add method currently uses sAdd(this.prefix, value) and
then expire(this.prefix, this.options.ttl), which applies TTL to the entire set
not individual members; change the data model and update the implementation so
TTL is per-member — either store each member as its own key with setex (use a
namespaced key derived from this.prefix and the member value, and setex using
this.options.ttl inside add/_add) or switch to a sorted set (use zAdd with
score=Date.now() and implement eviction via zRemRangeByScore based on now - ttl
in your add/_add and any lookup methods). Update any methods that read the set
(e.g., the public add, get, has helpers) to use the new structure (per-member
keys or sorted-set eviction) so the 24h staleness bound is enforced per entry
rather than by expiring the whole set.
In `@src/utils/once.ts`:
- Around line 22-27: The wrapper returned by the once helper currently sets
called = true then awaits fn(...args) but never rejects result if fn throws,
causing later callers to hang; change the invocation to catch errors and reject
the stored promise: when the returned async function is invoked, call
fn(...args) inside a try/catch, on success call result.resolve(value) and on
failure call result.reject(error) (and rethrow or return the rejection) so that
result is always settled; keep the existing called, result, and fn identifiers
and ensure result.reject is invoked in the catch branch.
---
Outside diff comments:
In `@src/lib/managed-commands/index.ts`:
- Around line 419-427: The /help <command> branch is querying this.getCommands()
directly and can return usage for commands the caller shouldn't see; replace the
direct this.getCommands() lookup with the same permission-filtered command list
used by the normal help flow (i.e., apply the existing permission-check helper
or the filtered-getter the codebase uses instead of this.getCommands()) before
finding the command, then call ManagedCommands.formatCommandUsage(cmd) and
ctx.reply as before; in short, perform the same permission filtering when
resolving the specific command as is used elsewhere so hidden/admin-only
commands are not returned.
---
Nitpick comments:
In `@src/utils/arrays.ts`:
- Around line 7-9: The current asyncMap function launches all promises at once
causing potential memory/IO pressure; update asyncMap to support bounded
concurrency by adding an optional concurrency parameter (e.g.,
asyncMap<T,U>(arr: T[], mapper: (item:T)=>Promise<U>, concurrency?: number)) or
create a new asyncMapWithConcurrency helper that runs at most N mapper tasks
concurrently; implement this by iterating the array and scheduling tasks through
a simple semaphore/worker queue or using a tiny internal pool (driven by the
provided concurrency value, defaulting to Infinity to preserve current
behavior), ensure the returned Promise resolves to U[] in original order and
reference the existing asyncMap symbol so callers can switch to the
concurrency-aware variant.
🪄 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: 5a839325-e753-42ce-adc4-d61b490a3a24
📒 Files selected for processing (7)
src/commands/index.tssrc/lib/managed-commands/command.tssrc/lib/managed-commands/index.tssrc/redis/set.tssrc/utils/arrays.tssrc/utils/once.tssrc/utils/types.ts
Comment on lines
+524
to
+535
| if (isAllowedInGroups(command)) { | ||
| const allowed = this.isCommandAllowedInGroup(command, ctx.chat.id) | ||
| if (!allowed) return false | ||
|
|
||
| if (command.permissions.allowGroupAdmins) { | ||
| const isAdmin = await isFromGroupAdmin() | ||
| if (isAdmin) return true | ||
| } | ||
| } | ||
|
|
||
| const roles = await this.getUserRoles(ctx.from.id, ctx) | ||
|
|
||
| // blacklist is stronger than whitelist | ||
| if (allowedRoles?.every((r) => !roles.includes(r))) return false | ||
| if (excludedRoles?.some((r) => roles.includes(r))) return false | ||
| const roles = await getUserRoles() | ||
| return this.isCommandAllowedForRoles(command, roles) |
Contributor
There was a problem hiding this comment.
Skip group-only permission logic when the current chat is private.
isAllowedInGroups(command) is also true for scope: "both", so this block still applies allowedGroupsId, excludedGroupsId, and allowGroupAdmins in private chats. That can incorrectly hide valid private commands and makes /help/execution diverge for commands that only use group-specific restrictions.
💡 Minimal fix
- if (isAllowedInGroups(command)) {
+ if ((ctx.chat.type === "group" || ctx.chat.type === "supergroup") && isAllowedInGroups(command)) {
const allowed = this.isCommandAllowedInGroup(command, ctx.chat.id)
if (!allowed) return false
if (command.permissions.allowGroupAdmins) {
const isAdmin = await isFromGroupAdmin()
if (isAdmin) return true
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/managed-commands/index.ts` around lines 524 - 535, The group-only
permission block is being applied for commands with scope "both" even in private
chats; update the logic in the permission check so that when the current chat is
private (check ctx.chat.type or equivalent), you skip the isAllowedInGroups
branch entirely — i.e., only run isAllowedInGroups(command) && isGroupChat check
before calling this.isCommandAllowedInGroup(command, ctx.chat.id) and evaluating
allowGroupAdmins via isFromGroupAdmin(); otherwise, directly proceed to
getUserRoles() and this.isCommandAllowedForRoles(command, roles).
Comment on lines
22
to
+27
| return async (...args: A) => { | ||
| if (!called) { | ||
| called = true | ||
| result = await fn(...args) | ||
| result.resolve(await fn(...args)) | ||
| } | ||
| return result | ||
| return await result |
Contributor
There was a problem hiding this comment.
A first-call rejection now deadlocks every later caller.
If fn(...args) throws or rejects at Line 25, called stays true but result is never resolved or rejected. After that, every invocation hangs on await result.
💡 Safer implementation
export function once<R, A extends unknown[]>(fn: (...args: A) => MaybePromise<R>) {
- const result: Awaiter<R> = new Awaiter()
- let called = false
-
- return async (...args: A) => {
- if (!called) {
- called = true
- result.resolve(await fn(...args))
- }
- return await result
- }
+ let result: Promise<Awaited<R>> | undefined
+
+ return (...args: A) => {
+ result ??= Promise.resolve(fn(...args))
+ return result
+ }
}📝 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
| return async (...args: A) => { | |
| if (!called) { | |
| called = true | |
| result = await fn(...args) | |
| result.resolve(await fn(...args)) | |
| } | |
| return result | |
| return await result | |
| export function once<R, A extends unknown[]>(fn: (...args: A) => MaybePromise<R>) { | |
| let result: Promise<Awaited<R>> | undefined | |
| return (...args: A) => { | |
| result ??= Promise.resolve(fn(...args)) | |
| return result | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/once.ts` around lines 22 - 27, The wrapper returned by the once
helper currently sets called = true then awaits fn(...args) but never rejects
result if fn throws, causing later callers to hang; change the invocation to
catch errors and reject the stored promise: when the returned async function is
invoked, call fn(...args) inside a try/catch, on success call
result.resolve(value) and on failure call result.reject(error) (and rethrow or
return the rejection) so that result is always settled; keep the existing
called, result, and fn identifiers and ensure result.reject is invoked in the
catch branch.
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.
Command menus (set with
setMyCommands) are cached and per-user, with global defaults/helpoutput is generated each time using the same permissions checksCaution
Might have broken permissions checks on execution - MUST CHECK THOROUGHLY