Open
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughAdds a Redis-backed set abstraction and per-user cached command-menu deduplication, a Redis-backed role cache and cache-first getUserRoles, expanded command-scope typings and helpers, permission-check caching and per-chat command registration, async array utilities, once behavior change, and new tests for permissions and once. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ManagedCommands
participant RedisSet as RedisSet<br/>(cachedUserSetCommands)
participant RolesCache as Redis<br/>(userRolesCache)
participant API as api.tg.permissions
participant Telegram as Telegram Bot API
Client->>ManagedCommands: Update received (userId, chatId, message)
ManagedCommands->>RedisSet: cachedUserSetCommands.has(userId:chatId)
alt cached (true)
RedisSet-->>ManagedCommands: true
ManagedCommands->>Telegram: Skip setMyCommands (no-op)
else not cached (false)
RedisSet-->>ManagedCommands: false
ManagedCommands->>RolesCache: get(userId)
alt roles cached
RolesCache-->>ManagedCommands: roles[]
else roles miss
RolesCache->>API: getRoles(userId)
API-->>RolesCache: roles[]
RolesCache->>RolesCache: cache roles (5m)
end
ManagedCommands->>ManagedCommands: compute allowed commands (scope + roles + admin)
ManagedCommands->>Telegram: setMyCommands(chat_member scoped)
Telegram-->>ManagedCommands: OK / error swallowed
ManagedCommands->>RedisSet: cachedUserSetCommands.add(userId:chatId, TTL 24h)
end
ManagedCommands-->>Client: Proceed (command handling / help / deny)
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
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
…ds` options defs and removed dependency on other files, `once` impl Co-authored-by: Copilot <copilot@github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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 | 🟡 Minor
/help <cmd>discloses usage for commands the user can't run.The lookup matches by trigger only — there's no permission check before replying with
formatCommandUsage. A regular user can probe for admin/role-gated triggers and read their argument schema/description. To stay consistent with the filtered listing below, gate the lookup bycheckPermissionsCachedand respond with the same "Command not found" message when the user lacks access (so existence isn't leaked either).🛠️ Suggested fix
- const [_, cmdArg] = text.replaceAll("/", "").split(" ") - if (cmdArg) { - const cmd = this.getCommands().find((c) => - Array.isArray(c.trigger) ? c.trigger.includes(cmdArg) : c.trigger === cmdArg - ) - if (!cmd) return ctx.reply(fmt(() => "Command not found. See /help for available commands.")) - - return ctx.reply(ManagedCommands.formatCommandUsage(cmd)) - } + const [_, cmdArg] = text.replaceAll("/", "").split(" ") + if (cmdArg) { + const cmd = this.getCommands().find((c) => + Array.isArray(c.trigger) ? c.trigger.includes(cmdArg) : c.trigger === cmdArg + ) + const notFound = fmt(() => "Command not found. See /help for available commands.") + if (!cmd) return ctx.reply(notFound) + const allowed = await this.checkPermissionsCached(cmd, ctx, getUserRoles, isFromGroupAdmin) + if (!allowed) return ctx.reply(notFound) + return ctx.reply(ManagedCommands.formatCommandUsage(cmd)) + }(Note:
getUserRoles/isFromGroupAdminwould need to be hoisted above theif (cmdArg)block.)🤖 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 <cmd> branch currently finds a command by matching its trigger (using getCommands() and cmdArg) and returns ManagedCommands.formatCommandUsage(cmd) without checking permissions; update this flow to call checkPermissionsCached with the same context (using getUserRoles and isFromGroupAdmin as needed) before replying so that if the user lacks access you return the same "Command not found" message via ctx.reply; essentially replace the unconditional reply with a permission-guarded branch that performs the cached permission check on the found cmd and only calls formatCommandUsage(cmd) when checkPermissionsCached grants access.
🧹 Nitpick comments (3)
tests/managed-commands/permissions.test.ts (1)
28-36: Redundant API transformer inplugins.
createDummyBotalready installs abot.api.config.usetransformer that records{method, payload}intooutgoingRequestsand short-circuits with{ ok: true, result: true }. Re-adding the same transformer through the conversationpluginsarray is at best dead code (whichever transformer wins short-circuits the other) and at worst a source of duplicate entries that would silently corruptlastSentText()/sendMessages()if execution order changes. Removing it makes the test setup simpler and avoids depending on transformer ordering inside grammy conversations.♻️ Suggested cleanup
- plugins: [ - async (ctx, next) => { - ctx.api.config.use(async (_, method, payload) => { - outgoingRequests.push({ method, payload }) - return { ok: true, result: true as ResultType } - }) - await next() - }, - ],(
ResultTypeimport can also be dropped if no longer used.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/managed-commands/permissions.test.ts` around lines 28 - 36, Remove the redundant API transformer added in the conversation `plugins` array: `async (ctx, next) => { ctx.api.config.use(async (_, method, payload) => { outgoingRequests.push({ method, payload }); return { ok: true, result: true as ResultType }; }); await next(); }`, because `createDummyBot` already installs a `bot.api.config.use` transformer that records `outgoingRequests` and short-circuits; delete that plugin entry (and remove the `ResultType` import if it becomes unused) so you don't get duplicate entries or rely on transformer ordering that can corrupt `lastSentText()`/`sendMessages()`.tests/common/dummy-bot.ts (1)
74-102: Consider distinguishingchat.idfromfrom.idin group helpers.
generateGroupCommandCall(trigger, id)andgenerateGroupMessage(text, id)use the sameidfor bothchat.idandfrom.id. In real Telegram traffic, group chat ids (typically negative supergroup ids) are unrelated to user ids, and several permission paths inManagedCommandsseparately readctx.chat.idandctx.from.id. Coupling them via a single arg risks a future regression that confuses the two staying green simply because the values happen to match.A small ergonomic tweak:
♻️ Optional
-export function generateGroupCommandCall(trigger: string, id: number = 0): Update { +export function generateGroupCommandCall(trigger: string, chatId: number = 0, userId: number = chatId): Update { return { update_id: 0, message: { text: `/${trigger}`, message_id: 0, date: Date.now(), entities: [{ type: "bot_command", offset: 0, length: trigger.length + 1 }], - chat: { id, title: "Test Group", type: "group" }, - from: { id, first_name: "Test", last_name: "Lastest", username: "testuser", is_bot: false }, + chat: { id: chatId, title: "Test Group", type: "group" }, + from: { id: userId, first_name: "Test", last_name: "Lastest", username: "testuser", is_bot: false }, }, } }Also applies to: 129-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/common/dummy-bot.ts` around lines 74 - 102, generateGroupCommandCall and generateGroupMessage currently use the same numeric arg for both chat.id and from.id which masks bugs; update these helpers so chat.id and from.id are distinct (either add a second parameter like userId or compute chat id as a distinct/negative id such as -100000 - id while keeping from.id positive) and adjust the calls in tests accordingly so group chat ids and user ids are different by default; references: generateGroupCommandCall, generateGroupMessage.src/lib/managed-commands/index.ts (1)
389-393: Avoid the-100probe chatId; check group restrictions explicitly.Passing
-100toisCommandAllowedInGroupto mean "no group restrictions" is fragile — it produces wrong results if a command happens to use-100inallowedGroupsId/excludedGroupsId, and it makes intent harder to read. Filter by the absence of any group-id restriction directly.♻️ Suggested refactor
- const groupCommands: BotCommand[] = freeCommands - .filter((cmd) => isAllowedInGroups(cmd) && this.isCommandAllowedInGroup(cmd, -100)) // only include commands that are allowed in all groups + const groupCommands: BotCommand[] = freeCommands + .filter( + (cmd) => + isAllowedInGroups(cmd) && + !cmd.permissions?.allowedGroupsId && + !cmd.permissions?.excludedGroupsId + ) .flatMap((cmd) => toBotCommands(cmd)) .concat([{ command: "help", description: "Show available commands" }])🤖 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 389 - 393, The filter currently uses a magic probe chatId (-100) when calling isCommandAllowedInGroup to mean "no group restrictions", which is fragile; update the groupCommands construction so you explicitly check that a command has no group-specific restrictions instead of calling isCommandAllowedInGroup(-100). Specifically, change the filter on freeCommands to include commands where isAllowedInGroups(cmd) is true AND the command has neither allowedGroupsId nor excludedGroupsId set (or their arrays are empty) before mapping with toBotCommands and adding the help command; reference the symbols groupCommands, freeCommands, isAllowedInGroups, isCommandAllowedInGroup, toBotCommands, allowedGroupsId, and excludedGroupsId when making the change.
🤖 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 89-93: The JSDoc comment above the cachedUserSetCommands property
contains a typo ("wether"); update the comment text to use the correct spelling
"whether" and ensure the description clearly reads: "A function to externally
cache whether a user has had the commands menu generated for them or not" so
that the doc for cachedUserSetCommands is accurate and typo-free.
In `@tests/common/dummy-bot.ts`:
- Line 48: The synthetic Telegram message timestamps are using Date.now()
(milliseconds) but Telegram's Message.date must be a Unix timestamp in seconds;
update the assignments in this file (e.g., the object with date: Date.now() and
corresponding code in generateGroupCommandCall, generateMessage, and
generateGroupMessage) to use seconds by converting Date.now() to
Math.floor(Date.now() / 1000) so message.date holds seconds instead of
milliseconds.
---
Outside diff comments:
In `@src/lib/managed-commands/index.ts`:
- Around line 419-427: The /help <cmd> branch currently finds a command by
matching its trigger (using getCommands() and cmdArg) and returns
ManagedCommands.formatCommandUsage(cmd) without checking permissions; update
this flow to call checkPermissionsCached with the same context (using
getUserRoles and isFromGroupAdmin as needed) before replying so that if the user
lacks access you return the same "Command not found" message via ctx.reply;
essentially replace the unconditional reply with a permission-guarded branch
that performs the cached permission check on the found cmd and only calls
formatCommandUsage(cmd) when checkPermissionsCached grants access.
---
Nitpick comments:
In `@src/lib/managed-commands/index.ts`:
- Around line 389-393: The filter currently uses a magic probe chatId (-100)
when calling isCommandAllowedInGroup to mean "no group restrictions", which is
fragile; update the groupCommands construction so you explicitly check that a
command has no group-specific restrictions instead of calling
isCommandAllowedInGroup(-100). Specifically, change the filter on freeCommands
to include commands where isAllowedInGroups(cmd) is true AND the command has
neither allowedGroupsId nor excludedGroupsId set (or their arrays are empty)
before mapping with toBotCommands and adding the help command; reference the
symbols groupCommands, freeCommands, isAllowedInGroups, isCommandAllowedInGroup,
toBotCommands, allowedGroupsId, and excludedGroupsId when making the change.
In `@tests/common/dummy-bot.ts`:
- Around line 74-102: generateGroupCommandCall and generateGroupMessage
currently use the same numeric arg for both chat.id and from.id which masks
bugs; update these helpers so chat.id and from.id are distinct (either add a
second parameter like userId or compute chat id as a distinct/negative id such
as -100000 - id while keeping from.id positive) and adjust the calls in tests
accordingly so group chat ids and user ids are different by default; references:
generateGroupCommandCall, generateGroupMessage.
In `@tests/managed-commands/permissions.test.ts`:
- Around line 28-36: Remove the redundant API transformer added in the
conversation `plugins` array: `async (ctx, next) => { ctx.api.config.use(async
(_, method, payload) => { outgoingRequests.push({ method, payload }); return {
ok: true, result: true as ResultType }; }); await next(); }`, because
`createDummyBot` already installs a `bot.api.config.use` transformer that
records `outgoingRequests` and short-circuits; delete that plugin entry (and
remove the `ResultType` import if it becomes unused) so you don't get duplicate
entries or rely on transformer ordering that can corrupt
`lastSentText()`/`sendMessages()`.
🪄 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: a1e8ca6c-ce6d-4bb0-8371-cad5bccafbda
📒 Files selected for processing (5)
src/lib/managed-commands/index.tssrc/utils/once.tstests/common/dummy-bot.tstests/managed-commands/permissions.test.tstests/once.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/once.ts
Co-authored-by: Copilot <copilot@github.com>
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