Repository navigation
fix(issue-90): restore observability and input validation in Telegram menu - #148
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrougheditMenu now only ignores Telegram’s benign "message is not modified" error and logs other edit failures; tryEditMenu was added to swallow edits. Cancellation mutations are separated from confirmation edits with logging. handleMenuCallback runs inside one recovery boundary, validates ChangesTelegram Menu Error Handling & Input Validation
Sequence Diagram(s)sequenceDiagram
participant User
participant BotHandler as MenuHandler
participant DB
participant TelegramAPI as Telegram
participant Logger
User->>BotHandler: callback (menu_pref_ttl/menu_pref_policy/menu_cancel_*)
BotHandler->>Logger: child(logger context)
BotHandler->>DB: lookup user
alt validation error
BotHandler->>Telegram: tryEditMenu("invalid ...")
BotHandler->>Logger: warn(payload ...)
else mutation path
BotHandler->>DB: expireIntent / cancelCard / savePref...
DB-->>BotHandler: success / error
alt DB error
BotHandler->>Logger: error(mutation failure ...)
BotHandler->>Telegram: tryEditMenu("Something went wrong")
else DB success
BotHandler->>Telegram: tryEditMenu("confirmation / updated prefs")
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/telegram/menuHandler.ts (2)
216-241:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSeparate the irreversible cancel from the Telegram confirmation edit.
Both handlers treat a post-success
editMessageTextfailure as if the cancel itself failed. If the card/intent is already cancelled and Telegram then returns 403/429/network errors, the catch logs the operation as failed and shows “Something went wrong”, which invites a retry against already-mutated state.Suggested split for the success side effect vs. notification path
async function doCancelIntent( bot: ReturnType<typeof getTelegramBot>, chatId: number | string, messageId: number, intentId: string, ): Promise<void> { try { await expireIntent(intentId); - const keyboard = new InlineKeyboard().text('⬅️ Back', 'menu_main:_'); - await editMenu( - bot, - chatId, - messageId, - '✅ Intent cancelled. Your budget has been returned.', - keyboard, - ); } catch (err) { log.error({ chatId, intentId, err }, 'doCancelIntent failed'); const keyboard = new InlineKeyboard().text('⬅️ Back', 'menu_main:_'); await editMenuSafe( bot, chatId, messageId, '⚠️ Something went wrong. Please try again.', keyboard, ); + return; + } + + const keyboard = new InlineKeyboard().text('⬅️ Back', 'menu_main:_'); + try { + await editMenu( + bot, + chatId, + messageId, + '✅ Intent cancelled. Your budget has been returned.', + keyboard, + ); + } catch (err) { + log.error({ chatId, intentId, err }, 'Intent cancelled, but confirmation edit failed'); } }Apply the same split to
doCancelCard.Also applies to: 373-392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/telegram/menuHandler.ts` around lines 216 - 241, The current doCancelIntent conflates the irreversible state change (expireIntent) with the Telegram UI update so any editMessageText failure is treated as a cancellation failure; change it to first call expireIntent and handle its errors (log and return/fail) and then in a separate try/catch attempt editMenu (and on error log only and optionally call editMenuSafe or notify user without reverting the cancellation), ensuring you do not treat edit failures as cancel failures; apply the same pattern to doCancelCard (separate the state mutation from the Telegram-side notification, use distinct try/catch blocks, and log errors accordingly).
453-455:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude the user lookup in the recovery boundary.
Line 454 still runs outside the outer
try, so a rejectedgetUserByChatId(chatId)bypasses the new logging and fallback message entirely. That leaves an unhandled DB-read failure on every action that requires a user record.Suggested change
- const user = await getUserByChatId(chatId); - - if (!user) { - await editMenu( - bot, - chatId, - messageId, - '⚠️ You need to sign up first. Send /start <code> to get started.', - ); - return; - } - - try { + let userId: string | undefined; + try { + const user = await getUserByChatId(chatId); + if (!user) { + await editMenu( + bot, + chatId, + messageId, + '⚠️ You need to sign up first. Send /start <code> to get started.', + ); + return; + } + userId = user.id; if (action === 'menu_balance') { await showBalance(bot, chatId, messageId, user); } else if (action === 'menu_history') { await showHistory(bot, chatId, messageId, user); @@ } catch (err) { log.error( - { chatId, messageId, action, payload, userId: user.id, err }, + { chatId, messageId, action, payload, userId, err }, 'menu callback handler failed', );Also applies to: 524-534
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/telegram/menuHandler.ts` around lines 453 - 455, The user lookup "const user = await getUserByChatId(chatId)" is executed outside the outer recovery try/catch so a rejected promise bypasses your error logging and fallback reply; move this await into the same outer try block (or wrap it in the same recovery try/catch) so failures are caught and handled by the existing recovery logging and fallback message code path, and apply the same change to the second occurrence around the getUserByChatId usage (the block covering the other action at the 524–534 area) so both user reads follow the same error-handling boundary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/telegram/menuHandler.test.ts`:
- Around line 588-773: Mock the '@/config/logger' export in these unit tests and
add assertions that the new observability calls are made: for failure paths in
handleMenuCallback (reference handleMenuCallback, mockExpireIntent,
mockCancelCard, and the show* handlers) assert logger.error was called with an
error for cancel/inner failures; for invalid TTL/policy and unknown menu_*
payloads assert logger.warn (or logger.error per implementation) is called with
a message indicating invalid payload/action; ensure the mocked logger is reset
between tests and add expect(...) checks immediately after invoking
handleMenuCallback in the existing "rejects ... without writing to the DB" and
cancel-failure tests to verify the correct logger method and that it received an
informative message.
---
Outside diff comments:
In `@src/telegram/menuHandler.ts`:
- Around line 216-241: The current doCancelIntent conflates the irreversible
state change (expireIntent) with the Telegram UI update so any editMessageText
failure is treated as a cancellation failure; change it to first call
expireIntent and handle its errors (log and return/fail) and then in a separate
try/catch attempt editMenu (and on error log only and optionally call
editMenuSafe or notify user without reverting the cancellation), ensuring you do
not treat edit failures as cancel failures; apply the same pattern to
doCancelCard (separate the state mutation from the Telegram-side notification,
use distinct try/catch blocks, and log errors accordingly).
- Around line 453-455: The user lookup "const user = await
getUserByChatId(chatId)" is executed outside the outer recovery try/catch so a
rejected promise bypasses your error logging and fallback reply; move this await
into the same outer try block (or wrap it in the same recovery try/catch) so
failures are caught and handled by the existing recovery logging and fallback
message code path, and apply the same change to the second occurrence around the
getUserByChatId usage (the block covering the other action at the 524–534 area)
so both user reads follow the same error-handling boundary.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 73365b41-04da-45aa-9206-62e6b3808964
📒 Files selected for processing (2)
src/telegram/menuHandler.tstests/unit/telegram/menuHandler.test.ts
georgyia
added a commit
that referenced
this pull request
May 3, 2026
recovery boundary, assert observability calls in tests CodeRabbit review on PR #148 surfaced three real issues: 1. doCancelIntent / doCancelCard conflated the irreversible state change with the Telegram confirmation edit. With bug 2's stricter editMenu that now propagates real failures, a 403/429/network error after the cancel succeeded would fall into the same catch and show "Something went wrong" — inviting the user to retry an already-cancelled intent or card. Both handlers are now structured as: mutation (try/catch → log + show error + return), then a separate fire-and-forget tryEditMenu for the confirmation. A Telegram failure on the success edit is logged but never surfaces a misleading error to the user. Renamed editMenuSafe → tryEditMenu for honesty (it does not throw; it does not "secure" anything). 2. getUserByChatId was awaited outside the outer recovery try/catch, so a Prisma failure on the user lookup bypassed the structured error log and the user-facing fallback. The whole dispatch — early shortcuts plus user lookup plus per-action handling — now lives inside one recovery boundary. 3. The unit tests asserted only the user-visible fallback string. The main contract of this PR is restored observability, so a regression to a bare catch could still pass. The tests now mock @/config/logger and assert log.warn / log.error were called with the expected payload and message for every new path: invalid TTL/policy/unknown action (warn), cancel-handler mutation failures (error), outer recovery (error), and the benign "message is not modified" case (no error log). Plus two new tests pinning the review's headline concern: when expireIntent / cancelCard succeed but the confirmation editMessageText fails, the user must NOT see "Something went wrong" and the handler must NOT log the operation as failed. Also fixes the `npm run format:check` CI failure on the previous commit. Tests: 43/43 menuHandler unit, 26/26 menuHandler integration, 375/375 total unit, 0 lint errors, prettier --check passes, dev server boots cleanly under docker.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/telegram/menuHandler.ts`:
- Around line 487-503: In the menu_pref_policy branch ensure we clear any stale
TTL when switching away from AFTER_TTL: after validating payload as
CardCancelPolicy, if policy === CardCancelPolicy.AFTER_TTL call showTtlPicker as
before, otherwise call savePrefPolicy in a way that also clears cardTtlMinutes
(e.g. pass null/undefined for cardTtlMinutes or update savePrefPolicy to set
cardTtlMinutes = NULL when policy !== AFTER_TTL). Update savePrefPolicy (or its
DB update logic) so it accepts/handles an explicit cardTtlMinutes value and
writes NULL to the users row when requested, preserving the invariant that
cardTtlMinutes is only set for AFTER_TTL.
- Around line 444-456: The cancel/card callback handlers currently accept an
untrusted intentId payload before resolving the Telegram user; resolve the
Telegram user first, validate ownership, and pass user.id into
showCancelConfirm, doCancelIntent, and doCancelCard so downstream
lookups/mutations are constrained by both intentId and userId. Update the
implementations of showCancelConfirm/ doCancelIntent/ doCancelCard (and any
called helpers like expireIntent or cancelCard) to accept a userId parameter and
use it in their DB queries/updates (e.g., WHERE intentId = ? AND userId = ?) to
ensure only the owner can view or modify the intent/card.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 6098a07e-2428-466b-941a-729d74a74e9c
📒 Files selected for processing (2)
src/telegram/menuHandler.tstests/unit/telegram/menuHandler.test.ts
7 tasks
6 tasks
JonasBaeumer
approved these changes
May 3, 2026
JonasBaeumer
left a comment
JonasBaeumer
left a comment
Owner
There was a problem hiding this comment.
LGTM, please address the open PR change and then proceed with merge. Well done @georgyia :)
JonasBaeumer
approved these changes
May 6, 2026
… menu
The menu system shipped with three silent-failure patterns that made
production debugging impossible and let forged callback payloads bypass
input validation:
1. Three bare `catch { }` blocks in `doCancelIntent`, `doCancelCard`, and
the outer `handleMenuCallback` discarded every error (IntentNotFound,
IllegalTransition, Stripe, DB, etc.) with no log line. Each is now a
bound `catch (err)` that logs at error level via the project pino
logger before showing the user-facing fallback.
2. `editMenu` swallowed every Telegram API error with `.catch(() => {})`,
masking 403 (bot blocked), 429 (rate limit), and network failures
alongside the benign 400 "message is not modified" response. It now
distinguishes the two: the benign case is silently ignored, real
failures are logged and propagated. A small `editMenuSafe` wrapper is
used in the outer error-recovery path so a failing fallback edit
doesn't crash the handler.
3. The TTL value parsed from `menu_pref_ttl` callback payloads was
written to the DB with no range check, so a forged
`menu_pref_ttl:-1` or `menu_pref_ttl:99999` bypassed the API-level
1..10080 bound enforced by Zod. The Telegram path now validates the
payload (integer-only string, within bounds) before calling
`savePrefTtl`. The same class of bug in `menu_pref_policy` (an
unchecked `payload as CardCancelPolicy` cast) is fixed at the same
time by validating against the enum.
Tests: 16 new unit tests in `tests/unit/telegram/menuHandler.test.ts`
covering the benign-vs-real Telegram error split, fallback safety when
the recovery edit also fails, the full TTL validation matrix (negative,
zero, too large, non-numeric, empty, fractional, and the three valid
boundary values), unknown policy rejection, and the bound-error
surfacing path for each cancel handler. All 39 menuHandler unit tests
and the existing menu integration tests pass.
Closes #90
recovery boundary, assert observability calls in tests CodeRabbit review on PR #148 surfaced three real issues: 1. doCancelIntent / doCancelCard conflated the irreversible state change with the Telegram confirmation edit. With bug 2's stricter editMenu that now propagates real failures, a 403/429/network error after the cancel succeeded would fall into the same catch and show "Something went wrong" — inviting the user to retry an already-cancelled intent or card. Both handlers are now structured as: mutation (try/catch → log + show error + return), then a separate fire-and-forget tryEditMenu for the confirmation. A Telegram failure on the success edit is logged but never surfaces a misleading error to the user. Renamed editMenuSafe → tryEditMenu for honesty (it does not throw; it does not "secure" anything). 2. getUserByChatId was awaited outside the outer recovery try/catch, so a Prisma failure on the user lookup bypassed the structured error log and the user-facing fallback. The whole dispatch — early shortcuts plus user lookup plus per-action handling — now lives inside one recovery boundary. 3. The unit tests asserted only the user-visible fallback string. The main contract of this PR is restored observability, so a regression to a bare catch could still pass. The tests now mock @/config/logger and assert log.warn / log.error were called with the expected payload and message for every new path: invalid TTL/policy/unknown action (warn), cancel-handler mutation failures (error), outer recovery (error), and the benign "message is not modified" case (no error log). Plus two new tests pinning the review's headline concern: when expireIntent / cancelCard succeed but the confirmation editMessageText fails, the user must NOT see "Something went wrong" and the handler must NOT log the operation as failed. Also fixes the `npm run format:check` CI failure on the previous commit. Tests: 43/43 menuHandler unit, 26/26 menuHandler integration, 375/375 total unit, 0 lint errors, prettier --check passes, dev server boots cleanly under docker.
When users switch away from AFTER_TTL, we now null out cardTtlMinutes to keep the DB state consistent with API validation and avoid stale TTL values.
georgyia
force-pushed
the
fix/issue-90-telegram-menu-observability
branch
from
May 13, 2026 18:18
0c22d2a to
c8524c8
Compare
Collaborator
Author
|
Rebased onto current Conflict resolution kept both sides:
Local verification:
|
5 tasks
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
2 participants
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.
Closes #90.
Summary
The Telegram menu system shipped with three silent-failure patterns that made production debugging impossible and let forged callback payloads bypass input validation. This PR restores observability and tightens the input boundary.
Bug 1 — Bare
catch { }blocks discarded every errordoCancelIntent,doCancelCard, and the outerhandleMenuCallbackeach usedcatch { }with no bound variable and no log line, hidingIntentNotFoundError,IllegalTransitionError, Stripe errors, DB errors, and everything else. In particular,doCancelIntentcould leave the user thinking their intent was cancelled when it had actually failed — with zero trace.Each is now
catch (err)with a structured pino log line aterrorlevel (includingchatId,intentId/action/payload,userId, and the error itself) before showing the user-facing fallback. The user-visible behaviour is unchanged.Bug 2 —
editMenuswallowed every Telegram API errorThe previous
.catch(() => {})masked 403 (bot blocked), 429 (rate limit), and network failures together with the benign 400 "message is not modified" response. Real Telegram failures became completely invisible.editMenunow distinguishes the two: the benign "message is not modified" 400 is silently ignored (matched by checkingerr.error_code === 400anddescription.includes('message is not modified')), while every other failure is logged and propagated. The outer error-recovery path uses a smalleditMenuSafewrapper so a failing recovery edit cannot crash the handler — the secondary failure is logged but not rethrown.Bug 3 — Unvalidated TTL/policy payloads from callback data
parseInt(payload, 10)was applied to themenu_pref_ttlcallback payload with no range check, so a crafted callback likemenu_pref_ttl:-1,menu_pref_ttl:99999, ormenu_pref_ttl:12.5bypassed the API-level 1..10080 bound enforced by the Zod schema insrc/api/routes/users.ts. The fix validates the payload as an integer-only string within the bound (regex^[1-9]\\d*$+ numeric range) before callingsavePrefTtl. Out-of-range values are logged at `warn` and surface a "Must be between 1 and 10080 minutes" message to the user.The same class of bug in
menu_pref_policy— an uncheckedpayload as CardCancelPolicycast — is fixed at the same time by checking the payload againstObject.values(CardCancelPolicy)and rejecting unknown values.Unknown
menu_*actions, previously silently ignored, are now logged at `warn` so a UI that ships a button the backend doesn't handle is visible in logs.Tests
tests/unit/telegram/menuHandler.test.tsgains 16 new unit tests in four describe blocks:expireIntentfailure,cancelCardfailure, and an inner DB failure all surface the user-facing fallback without rethrowing.All 39
menuHandlerunit tests pass (23 pre-existing + 16 new), all 371 unit tests pass, and the existing menu integration tests pass. The two pre-existing failures intests/integration/stripe/reconciliation.test.tsare unrelated (tracked in #88).Test plan
Out of scope
Summary by CodeRabbit
Bug Fixes
Tests