Add interactive wizard for /dispute (button-based order selection)#731
Add interactive wizard for /dispute (button-based order selection)#731Matobi98 wants to merge 10 commits intolnp2pBot:mainfrom
Conversation
…heck in dispute command.
|
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:
No actionable comments were generated in the recent review. 🎉 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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);
3 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