Add interactive wizard for /dispute (button-based order selection) - #731
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExtracts dispute initiation logic into a new handler Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@bot/modules/dispute/commands.ts`:
- Around line 116-122: In the conditional in the command handler, remove the
redundant "return" statements after the awaited calls—specifically eliminate the
trailing returns after messages.listOrdersForDispute(ctx, orders) and after
ctx.reply(ctx.i18n.t('you_have_no_orders'))—so the branch ends with the awaited
calls only; keep the if/else logic intact and ensure no other control flow needs
those returns.
In `@bot/modules/dispute/messages.ts`:
- Line 4: The file imports the symbol User but never uses it; remove User from
the import statement (the import line that currently reads "import { User } from
...") so the file no longer imports the unused symbol and run/verify linting to
ensure no remaining unused-import warnings.
🧹 Nitpick comments (3)
locales/it.yaml (1)
188-198: Consider translating remaining English strings inorder_detail.Several labels within
order_detailremain in English:
- Line 188:
Days in operation \\(buyer\\)- Line 190:
Successful trades \\(buyer\\)- Line 196:
Days in operation \\(seller\\)- Line 198:
Successful trades \\(seller\\)For consistency with the rest of the Italian locale, these should be translated.
bot/modules/dispute/actions.ts (1)
58-64: LGTM with minor suggestion.The implementation is clean and properly delegates to
handleDispute. For consistency withtakeDisputeabove (which has explicitPromise<void>return type), consider adding the same type annotation.Suggested improvement
-export const initiateDispute = async (ctx: MainContext) => { +export const initiateDispute = async (ctx: MainContext): Promise<void> => {bot/modules/dispute/messages.ts (1)
8-32: Consider simplifying the button mapping.The function works correctly, but
Promise.allwithasynccallback is unnecessary here since there are no async operations inside the map. This can be simplified:Suggested refactor
export const listOrdersForDispute = async ( ctx: MainContext, orders: IOrder[], ) => { try { - const buttons = await Promise.all( - orders.map(async order => { - return [ - { - text: `${order._id.toString().substring(0, 2)}..${order._id.toString().slice(-2)} - ${order.type} - ${order.fiat_code} ${order.fiat_amount}`, - callback_data: `initiateDispute_${order._id}`, - }, - ]; - }), - ); + const buttons = orders.map(order => [ + { + text: `${order._id.toString().substring(0, 2)}..${order._id.toString().slice(-2)} - ${order.type} - ${order.fiat_code} ${order.fiat_amount}`, + callback_data: `initiateDispute_${order._id}`, + }, + ]); await ctx.reply(ctx.i18n.t('choose_order_for_dispute'), {
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bot/modules/dispute/commands.ts (1)
23-31:⚠️ Potential issue | 🟠 MajorValidate
DISPUTE_START_WINDOWto avoid bypassing the time gate.
parseIntcan yieldNaN(or a negative value), which makes the comparison againstorder.taken_atfail open and allows immediate disputes. Validate the parsed value before using it.🔧 Suggested fix
- const disputStartWindow = process.env.DISPUTE_START_WINDOW; - if (disputStartWindow === undefined) + const disputeStartWindow = process.env.DISPUTE_START_WINDOW; + if (disputeStartWindow === undefined) throw new Error('DISPUTE_START_WINDOW environment variable not defined'); - const secsUntilDispute = parseInt(disputStartWindow); + const secsUntilDispute = Number.parseInt(disputeStartWindow, 10); + if (!Number.isFinite(secsUntilDispute) || secsUntilDispute < 0) { + throw new Error('DISPUTE_START_WINDOW must be a non-negative integer'); + } const time = new Date(); time.setSeconds(time.getSeconds() - secsUntilDispute);
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@bot/modules/dispute/commands.ts`:
- Around line 97-100: The conditional in the dispute command uses
ctx.state.command.args.length and then checks ctx.state.command.args[0] !==
'undefined' which tests the literal string "undefined" (redundant since length>0
proves an element exists); either remove the "'undefined'" comparison and rely
on the length check, or if you actually intend to treat the user typing the word
"undefined" as invalid, add a clarifying comment above this condition explaining
that special-case; update the condition in the handler that reads
ctx.state.command.args (the if block containing args.length and args[0] !==
'undefined') accordingly.
🧹 Nitpick comments (4)
bot/modules/dispute/commands.ts (4)
54-61: UsecountDocuments()instead of deprecatedcount().Mongoose 6.x deprecated
Model.count()in favor ofcountDocuments(). Since this project uses mongoose 6.13.6, update these calls to avoid deprecation warnings.Proposed fix
const buyerDisputes = - (await Dispute.count({ + (await Dispute.countDocuments({ $or: [{ buyer_id: buyer._id }, { seller_id: buyer._id }], })) + 1; const sellerDisputes = - (await Dispute.count({ + (await Dispute.countDocuments({ $or: [{ buyer_id: seller._id }, { seller_id: seller._id }], })) + 1;
38-38: Use strict equality or.equals()for ObjectId comparison.Comparing Mongoose ObjectIds with
==may produce unexpected results. Prefer===with.toString()or use the.equals()method for reliable comparison.Proposed fix
- if (user._id == order.buyer_id) initiator = 'buyer'; + if (user._id.equals(order.buyer_id)) initiator = 'buyer';
23-23: Minor typo in variable name.
disputStartWindowis missing an 'e' — should bedisputeStartWindowfor clarity.Proposed fix
- const disputStartWindow = process.env.DISPUTE_START_WINDOW; - if (disputStartWindow === undefined) + const disputeStartWindow = process.env.DISPUTE_START_WINDOW; + if (disputeStartWindow === undefined) throw new Error('DISPUTE_START_WINDOW environment variable not defined'); - const secsUntilDispute = parseInt(disputStartWindow); + const secsUntilDispute = parseInt(disputeStartWindow);
108-114: Inconsistent ObjectId query pattern — consider removing.toString().The query uses
user._id.toString()for comparisons, but throughout the codebase (bot/validations.ts, bot/start.ts, bot/ordersActions.ts, and bot/modules/orders/takeOrder.ts) all other seller_id and buyer_id queries useuser._iddirectly without conversion. For consistency, remove the explicit string conversion.Suggested change
const orders = await Order.find({ $or: [ - { seller_id: user._id.toString() }, - { buyer_id: user._id.toString() }, + { seller_id: user._id }, + { buyer_id: user._id }, ], status: { $in: ['ACTIVE', 'FIAT_SENT'] }, });
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bot/modules/dispute/commands.ts (1)
54-61:⚠️ Potential issue | 🟡 MinorReplace deprecated
Model.count()withcountDocuments().Mongoose 6.x deprecated
Model.count()in favor ofcountDocuments(). Since both usages here include filters, usecountDocuments()to avoid deprecation warnings.Proposed fix
const buyerDisputes = - (await Dispute.count({ + (await Dispute.countDocuments({ $or: [{ buyer_id: buyer._id }, { seller_id: buyer._id }], })) + 1; const sellerDisputes = - (await Dispute.count({ + (await Dispute.countDocuments({ $or: [{ buyer_id: seller._id }, { seller_id: seller._id }], })) + 1;
🧹 Nitpick comments (1)
bot/modules/dispute/commands.ts (1)
23-26: Typo in variable name:disputStartWindow→disputeStartWindow.Minor typo - missing 'e' in the variable name.
Proposed fix
- const disputStartWindow = process.env.DISPUTE_START_WINDOW; - if (disputStartWindow === undefined) + const disputeStartWindow = process.env.DISPUTE_START_WINDOW; + if (disputeStartWindow === undefined) throw new Error('DISPUTE_START_WINDOW environment variable not defined'); - const secsUntilDispute = parseInt(disputStartWindow); + const secsUntilDispute = parseInt(disputeStartWindow);
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Interactive wizard for /dispute
Overall Assessment
The feature is useful — letting users pick orders via buttons instead of memorizing order IDs reduces friction. However, there are several issues that need addressing before this can be merged.
🔴 Security: No authorization check on initiateDispute callback
In actions.ts, the initiateDispute handler extracts the order ID from the callback data and calls handleDispute() directly. But it does not verify that the user pressing the button is the same user who received the order list.
Telegram inline keyboard callbacks can technically be triggered by any user in certain scenarios (e.g., if the message is in a group or forwarded). While handleDispute internally validates the user against the order's buyer/seller, the ctx.deleteMessage() call happens BEFORE that validation. An unauthorized user pressing the button would delete someone else's message.
Recommendation: Move ctx.deleteMessage() after handleDispute() succeeds, or wrap it in a check that confirms the callback user matches the original message recipient.
🔴 Missing PENDING status in the order query
The dispute function queries orders with status ACTIVE or FIAT_SENT:
status: { $in: ['ACTIVE', 'FIAT_SENT'] },
But looking at validateDisputeOrder (which handleDispute calls), it allows disputes on orders with status ACTIVE, FIAT_SENT, and PENDING (among others depending on the validation logic). If a user has a stuck order in another disputable status, they won't see it in the button list but could still dispute it via /dispute . The interactive wizard should show the same orders that are actually disputable.
Recommendation: Align the status filter with whatever validateDisputeOrder accepts, or better yet, extract the disputable statuses into a shared constant.
🟡 Order display is too terse
The button text format is:
${order._id.toString().substring(0, 2)}..${order._id.toString().slice(-2)} - ${order.type} - ${order.fiat_code} ${order.fiat_amount}
This shows de..82 - sell - USD 100. For a user with multiple orders in the same currency and type, this is ambiguous. The first/last 2 chars of a MongoDB ObjectId are not unique enough to differentiate.
Recommendation: Show more context — at minimum the counterparty username and/or the creation date. Example:
sell 100 USD @counterparty (2h ago)
@mostronatorcoder
🟡 No pagination for users with many active orders
If a user has 20+ active orders (power traders in communities), the inline keyboard will have 20+ buttons. Telegram has limits on inline keyboard size (max 100 buttons, but UX degrades well before that).
Recommendation: Add a limit (e.g., 10 most recent) with a note that older orders can be disputed via /dispute .
🟡 Messy commit history
11 commits including:
"fixed bug in my implementation"
"fixing formatting"
"formatting"
"improved workarround"
Multiple merge commits from upstream
This should be squashed before merge. The PR description says "interactive wizard for /dispute" but the commit history tells a story of trial and error.
Recommendation: Squash to 1-2 clean commits before merging.
✅ What looks good
CI passes (lint, CodeQL, tests)
No conflicts with main
Localization done for 10 languages
The callback regex ^initiateDispute_([0-9a-f]{24})$ correctly validates MongoDB ObjectId format
Clean separation: handleDispute extracted as reusable, dispute wraps it with wizard logic
userMiddleware applied to the callback action
Verdict
Request Changes — The security concern with deleteMessage before authorization and the missing status alignment need fixing. The UX issues (terse display, no pagination) are also worth addressing but could be tracked as follow-up issues.
|
@mostronatorcoder
This makes no sense. Users cannot start a dispute on 'PENDING' orders, they can only dispute active orders (those with status 'FIAT_SENT' or 'ACTIVE'). |
|
@mostronatorcoder
This is not a realistic case, normally users have 1 or 2 active orders at the same time. |
|
@mostronatorcoder
we checked your suggestion, there is no way the user pressing the button is not the one who received the order list it as the bot only send the inline keyboard button to the user that executed the /dispute command. In the worst case scenario where the user just resent the message to an attacker, telegram does not allow forwarding buttons and |
|
@Luquitasjeffrey Thanks for the detailed responses. Let me address each point: 🔴 Missing PENDING statusYou're right — I reviewed 🟡 Order display / No paginationFair points — the display format is consistent with other commands like 🔴 Security: deleteMessage before authorizationI agree that in practice this is not exploitable because:
The theoretical risk I flagged doesn't apply in Telegram's private chat context. Dismissed. Given that all concerns have been addressed, I'll update my review to approve this PR. |
There was a problem hiding this comment.
All concerns from my previous review have been addressed by the maintainer's responses. The implementation is sound:
- Security is handled by
handleDispute()validation + Telegram's private chat model - Status filter correctly matches disputable statuses (
ACTIVE,FIAT_SENT) - Display format is consistent with existing commands (
/fiatsent,/release) - CI passes, no conflicts, localization complete
Approved ✅
4 participants
Adds an interactive wizard for /dispute so users can pick their order via buttons instead of typing /dispute , reducing errors and friction.
Summary by CodeRabbit