Upgrade mongoose version to 9.5.*#789
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request systematically updates MongoDB ObjectId handling across the codebase by converting Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
bot/validations.ts (1)
523-548:⚠️ Potential issue | 🔴 CriticalInconsistent ID handling —
seller_id/buyer_idqueries pass ObjectId against String schema fields without.toString().In
validateReleaseOrder(lines 524, 536) andvalidateUserWaitingOrder(lines 702, 707), the filters useuser._iddirectly:seller_id: user._id, // user._id is ObjectId buyer_id: user._id, // schema defines these as { type: String }But
models/order.tsdefines bothseller_idandbuyer_idas{ type: String }. In contrast, other validators in this same file (validateDisputeOrderline 574,validateFiatSentOrderline 600,validateSellerline 635) correctly call.toString().Mongoose 9 does not auto-cast ObjectId values to strings when querying String-typed schema fields. These four callsites will result in no matches —
findOneandfindwill silently returnnullor[], breaking thevalidateReleaseOrderrelease flow andvalidateUserWaitingOrderconcurrent-order guard, both critical user-facing paths.Proposed fix
- let where: any = { - seller_id: user._id, + let where: any = { + seller_id: user._id.toString(), status: 'WAITING_BUYER_INVOICE', _id: new ObjectId(orderId), }; @@ where = { $and: [ - { seller_id: user._id }, + { seller_id: user._id.toString() }, { $or: [ { status: 'ACTIVE' }, { status: 'FIAT_SENT' }, { status: 'DISPUTE' }, ], }, ], }; @@ - let where: any = { - seller_id: user._id, + let where: any = { + seller_id: user._id.toString(), status: 'WAITING_PAYMENT', }; @@ - where = { - buyer_id: user._id, + where = { + buyer_id: user._id.toString(), status: 'WAITING_BUYER_INVOICE', };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/validations.ts` around lines 523 - 548, The queries in validateReleaseOrder and validateUserWaitingOrder (where you set seller_id: user._id and buyer_id: user._id) pass an ObjectId into schema fields defined as String, causing no matches; update those query assignments to use user._id.toString() (e.g., set seller_id and buyer_id to user._id.toString()) so they match the String-typed fields, leaving the ObjectId(orderId) usage for _id unchanged and consistent with other validators like validateDisputeOrder and validateFiatSentOrder.bot/scenes.ts (1)
147-171:⚠️ Potential issue | 🟡 MinorInconsistent ID handling between query and insert in the same scene.
The
PendingPayment.findOnefilter at line 148 stringifiesorder._id, but thenew PendingPayment({...})call at lines 162–169 still storesuser_id: buyer._idandorder_id: order._idas rawObjectIdvalues. Going forward, the lookup and the persist path should agree, otherwise this duplicate‑payment guard can drift if the schema field types are tightened, and it diverges from the convention now applied acrossbot/ordersActions.ts,bot/modules/orders/takeOrder.ts, and the dispute/block flows.🐛 Suggested fix
const pp = new PendingPayment({ amount: order.amount, payment_request: lnInvoice, - user_id: buyer._id, + user_id: buyer._id.toString(), description: order.description, hash: order.hash, - order_id: order._id, + order_id: order._id.toString(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/scenes.ts` around lines 147 - 171, The lookup uses a stringified order id (PendingPayment.findOne with order._id.toString()) but the insert constructs PendingPayment with raw ObjectId values (new PendingPayment using user_id: buyer._id and order_id: order._id), causing inconsistent ID types; update the creation path in the block that constructs the PendingPayment (the new PendingPayment instantiation and saved order.paid_hold_buyer_invoice_updated logic) to store the same string form used in the query (e.g., use order._id.toString() and buyer._id.toString() for order_id and user_id) so lookup and persist agree with PendingPayment.findOne.bot/messages.ts (1)
1143-1143:⚠️ Potential issue | 🟡 MinorMissed
.toString()fororderIdhere — inconsistent with the rest of the PR.Other call sites of the
setinvoice_cmd_ordertemplate were normalized toorder._id.toString()(e.g., Line 294), butnotLightningInvoiceMessagestill passes the raworder._id. Behaviorally the i18n engine will stringify it, but please make this consistent so the model-level_id: Types.ObjectIdtyping is respected uniformly.🔧 Proposed fix
- ctx.i18n.t('setinvoice_cmd_order', { orderId: order._id }), + ctx.i18n.t('setinvoice_cmd_order', { orderId: order._id.toString() }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/messages.ts` at line 1143, The i18n call in notLightningInvoiceMessage passes order._id directly which breaks consistency with other uses; update the ctx.i18n.t invocation that references 'setinvoice_cmd_order' to pass order._id.toString() instead of order._id so the model-level Types.ObjectId is normalized (look for the ctx.i18n.t('setinvoice_cmd_order', { orderId: order._id }) call in notLightningInvoiceMessage and replace the orderId value with order._id.toString()).bot/start.ts (1)
103-140:⚠️ Potential issue | 🔴 CriticalUse
QueryFilter<IOrder>instead ofany— Mongoose 9.5 broke withFilterQuery.These filter objects currently lose type safety with
any. With Mongoose 9.5.0 (in package.json), usemongoose.QueryFilter<IOrder>to restore schema-aware typing. Note thatFilterQuery<T>was removed in Mongoose 9.0; the new public type isQueryFilter<T>, which properly supports$and/$oroperators. This catch typos in field names at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/start.ts` around lines 103 - 140, Replace the loose-typed "where: any" filter objects with Mongoose's schema-aware type by declaring them as QueryFilter<IOrder> (instead of any) wherever you build query filters in this handler (e.g., the "where" variables used before Order.find(...) in the branches for the default, '/fiatsent', '/release', and '/setinvoice' commands); import QueryFilter from mongoose and the IOrder interface/type from your models/typings, update each "where" declaration to use QueryFilter<IOrder>, and ensure the typed filters still compile with the $and/$or/$in operators used so field-name typos are caught at compile time.
🧹 Nitpick comments (9)
db_connect.ts (1)
18-18: Drop the empty options object and the unnecessary cast.Passing
{} as ConnectOptionsis a no-op and the cast hides a real type problem if one ever arises. With the deprecateduseNewUrlParser/useUnifiedTopologyremoved, just callmongoose.connect(MONGO_URI)and remove the now-unusedConnectOptionsimport.Proposed diff
-import mongoose, { ConnectOptions } from 'mongoose'; +import mongoose from 'mongoose'; @@ const connect = () => { - mongoose.connect(MONGO_URI, {} as ConnectOptions); + mongoose.connect(MONGO_URI); return mongoose; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db_connect.ts` at line 18, Remove the unnecessary cast and empty options when connecting to MongoDB: replace the call to mongoose.connect(MONGO_URI, {} as ConnectOptions) with mongoose.connect(MONGO_URI) and delete the now-unused ConnectOptions import; ensure you update any imports or references so only MONGO_URI and mongoose are used in db_connect.ts.bot/validations.ts (3)
25-25: Drop the leftover migration breadcrumb.
// Removed redundant ObjectId declarationdocuments a transient cleanup, not the code. Please remove it before merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/validations.ts` at line 25, Remove the stray migration breadcrumb comment "// Removed redundant ObjectId declaration" from bot/validations.ts; locate that exact comment string in the file and delete it so only actual code and meaningful comments remain before merging.
605-607: Inconsistent_idfilter construction.Other branches in this file build the
_idfilter asnew ObjectId(orderId)(lines 526, 548, 572). Here it's a raw string, which works thanks to Mongoose's path casting but is a stylistic outlier and easy to miss when refactoring.- if (orderId) { - where._id = orderId; - } + if (orderId) { + where._id = new ObjectId(orderId); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/validations.ts` around lines 605 - 607, The _id filter is being set as a raw string in the conditional (where._id = orderId) which is inconsistent with other branches that use new ObjectId(orderId); update that assignment to construct an ObjectId (i.e., where._id = new ObjectId(orderId)) using the same ObjectId constructor used in the other branches (see the usages around lines building _id with new ObjectId) so the filter style matches and avoid refactor hazards.
523-523: Regression in query typing:FilterQuery<OrderQuery>→any.The
whereobjects were previously typed (per the AI summary) and are nowany. That weakens type safety exactly where it matters most for this PR — the Mongoose 9 cast/typing changes are the kind of thing those types would have caught (e.g. theseller_id: user._idissue flagged above). Consider keeping the typed form, e.g.FilterQuery<IOrder>, even if you have to add a small union/intersection to allow the$and/$orshape.Also applies to: 598-598, 701-701
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/validations.ts` at line 523, The local query variable `where` was loosened to `any`, regressing type-safety; change its type back to a Mongoose FilterQuery for orders and allow the $and/$or compound shapes—e.g. replace `let where: any = {` with something like `let where: FilterQuery<IOrder> & ({ $and?: FilterQuery<IOrder>[]; $or?: FilterQuery<IOrder>[] }) = {` (and apply the same typed change at the other occurrences flagged around the file), so you retain strict typing for fields while permitting compound operators.bot/modules/orders/scenes.ts (1)
78-78: Optional chaining reads cleaner here.
community && community._id.toString()will pass throughnull/undefined/false. Optional chaining is more idiomatic for ID derivation and yieldsundefinedconsistently whencommunityis missing.♻️ Suggested refactor
- community_id: community && community._id.toString(), + community_id: community?._id.toString(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/modules/orders/scenes.ts` at line 78, Replace the brittle "community && community._id.toString()" expression with optional chaining to consistently yield undefined when community is missing; update the property to use "community?._id?.toString()" (reference: the community_id assignment in the object where community is referenced).jobs/pending_payments.ts (1)
13-15: ExportgetPaymentAttemptsto eliminate the same logic duplicated across 3+ files.The same
Number(process.env.PAYMENT_ATTEMPTS)pattern exists in:
bot/start.ts:1018-1021bot/modules/community/messages.ts:161bot/modules/community/scenes.ts:949Hoist this helper (e.g., into
util/index.ts) and reuse it everywhere so the default and coercion behavior stay consistent.Also note:
Number(env) || 2will return2whenPAYMENT_ATTEMPTS="0". That edge case is benign here, but consider explicitNumber.isFinite(...) ? n : 2if you want the fallback to apply only when the env var is unset/invalid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jobs/pending_payments.ts` around lines 13 - 15, Export the getPaymentAttempts helper so the env-parsing logic is centralized and reused instead of duplicated; move or re-create getPaymentAttempts in util/index.ts, export it, and replace the three ad-hoc uses (getPaymentAttempts in jobs/pending_payments.ts plus the calls in bot/start.ts, bot/modules/community/messages.ts, and bot/modules/community/scenes.ts) to import and call the shared function; update the implementation to use an explicit check like const n = Number(process.env.PAYMENT_ATTEMPTS); return Number.isFinite(n) ? n : 2 so PAYMENT_ATTEMPTS="0" yields 0 while invalid/absent values fall back to 2.bot/messages.ts (3)
752-757: Duplicated community-i18n construction; consider extracting a helper.Lines 752-757 and 805-810 are identical and run on every published order, paying the cost of
new I18n({ directory: 'locales' })(synchronous file load) each time. Extract a small helper (e.g.,getCommunityI18nContext(language)) and ideally cacheI18ninstances per language to avoid reloading locale files on every publish.♻️ Sketch
const communityI18nCache = new Map<string, I18nContext>(); const getCommunityI18nContext = (language: string): I18nContext => { let ctx = communityI18nCache.get(language); if (!ctx) { ctx = new I18n({ defaultLanguageOnMissing: true, defaultLanguage: language, directory: 'locales', }).createContext(language, {}); communityI18nCache.set(language, ctx); } return ctx; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/messages.ts` around lines 752 - 757, The duplicated construction of communityI18n (new I18n({...}).createContext(...)) should be extracted into a helper like getCommunityI18nContext(language) that caches I18n contexts per language (e.g., a Map<string, I18nContext>) so you reuse the same context instead of reloading locale files on every publish; replace both occurrences that set communityI18n with calls to getCommunityI18nContext(community.language) and ensure the helper initializes the I18n with defaultLanguageOnMissing, defaultLanguage and directory exactly as currently used.
749-757: Remove the redundantnew ObjectId(...)cast and the!assertion onorder.community_idin both publishBuy and publishSell functions.
order.community_idis already narrowed to truthy by theif (order.community_id)check, and Mongoose automatically casts string or Types.ObjectId values for the_idfilter, so wrapping innew ObjectId(...)is unnecessary. The same duplicate pattern appears at lines 802–810.Additionally,
defaultLanguageand the second argument tocreateContextboth receivecommunity.language—pass the language once:♻️ Proposed fix
- const community = await Community.findOne({ _id: new ObjectId(order.community_id!) }); + const community = await Community.findOne({ _id: order.community_id }); if (community && community.language) { communityI18n = new I18n({ defaultLanguageOnMissing: true, defaultLanguage: community.language, directory: 'locales', - }).createContext(community.language, {}); + }).createContext({});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/messages.ts` around lines 749 - 757, Remove the unnecessary ObjectId cast and non-null assertion and avoid passing the community language twice: in both publishBuy and publishSell replace Community.findOne({ _id: new ObjectId(order.community_id!) }) with Community.findOne({ _id: order.community_id }) (no casting or "!"), and when constructing communityI18n keep defaultLanguage: community.language but call createContext with only the language argument (e.g., .createContext(community.language)) so the language value isn't duplicated.
1207-1995: Standardizeorder._idto.toString()when passing toi18n.t()for consistency.The file mixes stringified and unstringified usage (22 instances without
.toString()vs. 8 with it), despiteorder._idbeing typed asmongoose.Types.ObjectId. While runtime behavior is unchanged—i18n auto-stringifies—standardizing across all 22+ call sites would improve code consistency and clarity. This pattern extends throughout thebot/directory (25 unstringified vs. 9 stringified occurrences), suggesting a broader refactoring opportunity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/messages.ts` around lines 1207 - 1995, Multiple places in this file pass order._id into i18n.t() without explicitly stringifying it; standardize all such usages to use order._id.toString() for consistency. Search for functions that interpolate order._id (e.g., successCancelOrderMessage, counterPartyCancelOrderMessage, okCooperativeCancelMessage, wizardAddInvoiceExitMessage, expiredOrderMessage, toBuyerPendingPaymentFailedMessage, toAdminChannelPendingPaymentSuccessMessage, toAdminChannelPendingPaymentFailedMessage and any other helpers in this file) and replace the argument passed to i18n.t() from order._id to order._id.toString() everywhere it appears. Ensure you only change the interpolation value (keep surrounding i18n keys and options intact) and run tests/lint to confirm no type issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bot/commands.ts`:
- Line 751: The call to setCooperativeCancelFlag currently wraps order._id with
new ObjectId(...) and casts to any; remove the unsafe cast and pass a string
instead (e.g., use order._id.toString() or pass order._id directly if already a
string) so the call becomes setCooperativeCancelFlag(order._id.toString(),
initiator); update callers to stop using "as any" and, optionally, remove the
redundant new ObjectId(orderId) conversion inside setCooperativeCancelFlag if
you want to rely on Mongoose auto-casting.
- Around line 1-3: Move the in-module declaration so all imports come first:
place "const { ObjectId } = mongoose.Types;" after the import statements (below
the imports of mongoose and the named validators like validateFiatSentOrder and
validateReleaseOrder) so that ObjectId is still derived from mongoose but no
longer sits between import lines, resolving the import/first ESLint error.
- Line 685: The call currently constructs and casts an ObjectId around orderId
which bypasses ordersActions.getOrder's own validation; update both occurrences
(the call at the order retrieval near the earlier block and the second similar
call) to pass the raw string orderId directly to ordersActions.getOrder so its
ObjectId.isValid(orderId) check and user-facing notValidIdMessage remain
effective (remove new ObjectId(orderId) as any and pass orderId: string to
getOrder).
- Line 142: Remove the unnecessary new ObjectId(...) wrappings around
string-typed ID fields (e.g. places calling Order.findOne({ _id: new
ObjectId(orderId) }) and User.findOne({ _id: new ObjectId(order.buyer_id!) }))
because Mongoose 9 auto-casts strings; instead pass the raw string IDs to
findOne. Replace occurrences that wrap order.buyer_id, order.seller_id, orderId,
etc., with the plain values and add explicit null/undefined guards where the
field is typed string | null (for example, if (!order.buyer_id) return; const
buyer = await User.findOne({ _id: order.buyer_id });). Update all listed call
sites in bot/commands.ts (and similar patterns in bot/validations.ts,
jobs/pending_payments.ts, bot/messages.ts) to remove new ObjectId and drop
non-null assertions so runtime nulls are handled explicitly.
In `@bot/modules/community/messages.ts`:
- Around line 159-163: The guard using Number(process.env.PAYMENT_ATTEMPTS) can
become NaN and bypass the duplicate-payment check; replace that ad-hoc
conversion by importing and using the shared getPaymentAttempts() helper from
jobs/pending_payments.ts (export it if not already exported) and use it in the
PendingPayment.findOne query (the same change should be applied to
bot/modules/community/scenes.ts around the withdrawal logic and bot/start.ts
where PAYMENT_ATTEMPTS is used) so attempts uses a validated numeric fallback
(e.g., || 2) instead of Number(process.env.PAYMENT_ATTEMPTS).
In `@bot/scenes.ts`:
- Line 149: The filter uses Number(process.env.PAYMENT_ATTEMPTS) which can
become NaN and make the $lt check always fail; define and validate a
PAYMENT_ATTEMPTS constant at startup (like the existing
HOLD_INVOICE_EXPIRATION_WINDOW pattern) by parsing process.env.PAYMENT_ATTEMPTS
(e.g., parseInt) and throwing/failing fast if it is missing or NaN, then replace
Number(process.env.PAYMENT_ATTEMPTS) in the attempts: { $lt: ... } expression
with that validated PAYMENT_ATTEMPTS constant so the duplicate‑payment guard
works reliably.
In `@bot/start.ts`:
- Around line 6-7: Remove the unused destructure "ObjectId" (and the "mongoose"
import if it's only present to obtain ObjectId) from this module: locate the
import line that reads "import mongoose from 'mongoose'; const { ObjectId } =
mongoose.Types;" and delete the ObjectId destructure (and the whole mongoose
import if no other symbol from mongoose is referenced elsewhere in the file);
ensure any existing code that uses _id.toString() or string comparisons
continues to work without ObjectId, adjust imports/types if necessary, and run
ESLint to confirm the warning is resolved.
In `@jobs/pending_payments.ts`:
- Line 28: Remove unnecessary use of new ObjectId(...) when querying (e.g.,
Order.findOne({ _id: new ObjectId(pending.order_id) }) and other similar
queries) and instead pass the string directly so Mongoose can auto-cast;
additionally, replace non-null assertions on nullable fields buyer_id and
seller_id with explicit null checks and proper error handling before querying
(e.g., verify order.buyer_id and order.seller_id are present and throw or return
a clear error if missing), then call User.findOne({ _id: order.buyer_id }) /
User.findOne({ _id: order.seller_id }) and handle the case where the returned
buyerUser or sellerUser is null.
In `@package.json`:
- Line 31: package.json currently depends on "mongoose": "^9.5.0" which requires
Node.js >=20.19.0 but the package.json "engines.node" declares >=18.0.0; update
the "engines.node" constraint to ">=20.19.0" and align your CI workflow (node
version in CI config) to use >=20.19.0, then run/author regression tests around
code paths using mongoose (and review mongoose 9.x migration notes) to catch
breaking changes before merging; look for the "mongoose" dependency and the
"engines.node" field to apply these changes.
---
Outside diff comments:
In `@bot/messages.ts`:
- Line 1143: The i18n call in notLightningInvoiceMessage passes order._id
directly which breaks consistency with other uses; update the ctx.i18n.t
invocation that references 'setinvoice_cmd_order' to pass order._id.toString()
instead of order._id so the model-level Types.ObjectId is normalized (look for
the ctx.i18n.t('setinvoice_cmd_order', { orderId: order._id }) call in
notLightningInvoiceMessage and replace the orderId value with
order._id.toString()).
In `@bot/scenes.ts`:
- Around line 147-171: The lookup uses a stringified order id
(PendingPayment.findOne with order._id.toString()) but the insert constructs
PendingPayment with raw ObjectId values (new PendingPayment using user_id:
buyer._id and order_id: order._id), causing inconsistent ID types; update the
creation path in the block that constructs the PendingPayment (the new
PendingPayment instantiation and saved order.paid_hold_buyer_invoice_updated
logic) to store the same string form used in the query (e.g., use
order._id.toString() and buyer._id.toString() for order_id and user_id) so
lookup and persist agree with PendingPayment.findOne.
In `@bot/start.ts`:
- Around line 103-140: Replace the loose-typed "where: any" filter objects with
Mongoose's schema-aware type by declaring them as QueryFilter<IOrder> (instead
of any) wherever you build query filters in this handler (e.g., the "where"
variables used before Order.find(...) in the branches for the default,
'/fiatsent', '/release', and '/setinvoice' commands); import QueryFilter from
mongoose and the IOrder interface/type from your models/typings, update each
"where" declaration to use QueryFilter<IOrder>, and ensure the typed filters
still compile with the $and/$or/$in operators used so field-name typos are
caught at compile time.
In `@bot/validations.ts`:
- Around line 523-548: The queries in validateReleaseOrder and
validateUserWaitingOrder (where you set seller_id: user._id and buyer_id:
user._id) pass an ObjectId into schema fields defined as String, causing no
matches; update those query assignments to use user._id.toString() (e.g., set
seller_id and buyer_id to user._id.toString()) so they match the String-typed
fields, leaving the ObjectId(orderId) usage for _id unchanged and consistent
with other validators like validateDisputeOrder and validateFiatSentOrder.
---
Nitpick comments:
In `@bot/messages.ts`:
- Around line 752-757: The duplicated construction of communityI18n (new
I18n({...}).createContext(...)) should be extracted into a helper like
getCommunityI18nContext(language) that caches I18n contexts per language (e.g.,
a Map<string, I18nContext>) so you reuse the same context instead of reloading
locale files on every publish; replace both occurrences that set communityI18n
with calls to getCommunityI18nContext(community.language) and ensure the helper
initializes the I18n with defaultLanguageOnMissing, defaultLanguage and
directory exactly as currently used.
- Around line 749-757: Remove the unnecessary ObjectId cast and non-null
assertion and avoid passing the community language twice: in both publishBuy and
publishSell replace Community.findOne({ _id: new ObjectId(order.community_id!)
}) with Community.findOne({ _id: order.community_id }) (no casting or "!"), and
when constructing communityI18n keep defaultLanguage: community.language but
call createContext with only the language argument (e.g.,
.createContext(community.language)) so the language value isn't duplicated.
- Around line 1207-1995: Multiple places in this file pass order._id into
i18n.t() without explicitly stringifying it; standardize all such usages to use
order._id.toString() for consistency. Search for functions that interpolate
order._id (e.g., successCancelOrderMessage, counterPartyCancelOrderMessage,
okCooperativeCancelMessage, wizardAddInvoiceExitMessage, expiredOrderMessage,
toBuyerPendingPaymentFailedMessage, toAdminChannelPendingPaymentSuccessMessage,
toAdminChannelPendingPaymentFailedMessage and any other helpers in this file)
and replace the argument passed to i18n.t() from order._id to
order._id.toString() everywhere it appears. Ensure you only change the
interpolation value (keep surrounding i18n keys and options intact) and run
tests/lint to confirm no type issues.
In `@bot/modules/orders/scenes.ts`:
- Line 78: Replace the brittle "community && community._id.toString()"
expression with optional chaining to consistently yield undefined when community
is missing; update the property to use "community?._id?.toString()" (reference:
the community_id assignment in the object where community is referenced).
In `@bot/validations.ts`:
- Line 25: Remove the stray migration breadcrumb comment "// Removed redundant
ObjectId declaration" from bot/validations.ts; locate that exact comment string
in the file and delete it so only actual code and meaningful comments remain
before merging.
- Around line 605-607: The _id filter is being set as a raw string in the
conditional (where._id = orderId) which is inconsistent with other branches that
use new ObjectId(orderId); update that assignment to construct an ObjectId
(i.e., where._id = new ObjectId(orderId)) using the same ObjectId constructor
used in the other branches (see the usages around lines building _id with new
ObjectId) so the filter style matches and avoid refactor hazards.
- Line 523: The local query variable `where` was loosened to `any`, regressing
type-safety; change its type back to a Mongoose FilterQuery for orders and allow
the $and/$or compound shapes—e.g. replace `let where: any = {` with something
like `let where: FilterQuery<IOrder> & ({ $and?: FilterQuery<IOrder>[]; $or?:
FilterQuery<IOrder>[] }) = {` (and apply the same typed change at the other
occurrences flagged around the file), so you retain strict typing for fields
while permitting compound operators.
In `@db_connect.ts`:
- Line 18: Remove the unnecessary cast and empty options when connecting to
MongoDB: replace the call to mongoose.connect(MONGO_URI, {} as ConnectOptions)
with mongoose.connect(MONGO_URI) and delete the now-unused ConnectOptions
import; ensure you update any imports or references so only MONGO_URI and
mongoose are used in db_connect.ts.
In `@jobs/pending_payments.ts`:
- Around line 13-15: Export the getPaymentAttempts helper so the env-parsing
logic is centralized and reused instead of duplicated; move or re-create
getPaymentAttempts in util/index.ts, export it, and replace the three ad-hoc
uses (getPaymentAttempts in jobs/pending_payments.ts plus the calls in
bot/start.ts, bot/modules/community/messages.ts, and
bot/modules/community/scenes.ts) to import and call the shared function; update
the implementation to use an explicit check like const n =
Number(process.env.PAYMENT_ATTEMPTS); return Number.isFinite(n) ? n : 2 so
PAYMENT_ATTEMPTS="0" yields 0 while invalid/absent values fall back to 2.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e08fa77d-fbf5-43ea-829e-36c885d14a8c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
bot/commands.tsbot/messages.tsbot/modules/block/commands.tsbot/modules/community/commands.tsbot/modules/community/messages.tsbot/modules/community/scenes.tsbot/modules/dispute/actions.tsbot/modules/dispute/commands.tsbot/modules/nostr/events.tsbot/modules/orders/commands.tsbot/modules/orders/messages.tsbot/modules/orders/scenes.tsbot/modules/orders/takeOrder.tsbot/ordersActions.tsbot/scenes.tsbot/start.tsbot/validations.tsdb_connect.tsjobs/pending_payments.tsmodels/community.tsmodels/order.tsmodels/user.tspackage.jsontsconfig.jsonutil/communityHelper.tsutil/index.ts
| import mongoose from 'mongoose'; | ||
| const { ObjectId } = mongoose.Types; | ||
| import { validateFiatSentOrder, validateReleaseOrder } from './validations'; |
There was a problem hiding this comment.
Fix ESLint import/first failure breaking CI.
Pipeline reports: ESLint (import/first): Import in body of module; reorder to top. The const { ObjectId } = mongoose.Types; declaration on line 2 sits between two import statements; move it below all imports.
As per coding guidelines: "address warnings instead of disabling them".
🔧 Proposed fix
import mongoose from 'mongoose';
-const { ObjectId } = mongoose.Types;
import { validateFiatSentOrder, validateReleaseOrder } from './validations';
import {
createHoldInvoice,
@@ -27,6 +26,8 @@
import { HasTelegram, MainContext } from './start';
import { CommunityContext } from './modules/community/communityContext';
+const { ObjectId } = mongoose.Types;
+
enum UserOrderRole {📝 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.
| import mongoose from 'mongoose'; | |
| const { ObjectId } = mongoose.Types; | |
| import { validateFiatSentOrder, validateReleaseOrder } from './validations'; | |
| import mongoose from 'mongoose'; | |
| import { validateFiatSentOrder, validateReleaseOrder } from './validations'; |
🧰 Tools
🪛 ESLint
[error] 3-3: Import in body of module; reorder to top.
(import/first)
🪛 GitHub Actions: Auto Check Lint
[error] 3-3: ESLint (import/first): Import in body of module; reorder to top.
🪛 GitHub Check: Lint
[failure] 3-3:
Import in body of module; reorder to top
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bot/commands.ts` around lines 1 - 3, Move the in-module declaration so all
imports come first: place "const { ObjectId } = mongoose.Types;" after the
import statements (below the imports of mongoose and the named validators like
validateFiatSentOrder and validateReleaseOrder) so that ObjectId is still
derived from mongoose but no longer sits between import lines, resolving the
import/first ESLint error.
| const orderId = (ctx.update as any).callback_query.message.text; | ||
| if (!orderId) return; | ||
| order = await Order.findOne({ _id: orderId }); | ||
| order = await Order.findOne({ _id: new ObjectId(orderId) }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inventory all `new ObjectId(...)` usages introduced in the PR for cross-file consistency review.
rg -nP '\bnew\s+ObjectId\s*\(' --type=ts -C1Repository: lnp2pBot/bot
Length of output: 8827
🏁 Script executed:
fd -t f '\.ts$' -path '*/models/*' | head -20Repository: lnp2pBot/bot
Length of output: 225
🏁 Script executed:
rg -l "interface IOrder|class Order|model.*Order" --type=ts -m 5Repository: lnp2pBot/bot
Length of output: 92
🏁 Script executed:
# Find Order model definition
git ls-files | grep -i 'order' | grep -E 'models|schema' | head -10Repository: lnp2pBot/bot
Length of output: 72
🏁 Script executed:
cat -n models/order.tsRepository: lnp2pBot/bot
Length of output: 6007
🏁 Script executed:
rg -l "interface IUser|export.*User|model.*User" --type=ts models/ | head -5Repository: lnp2pBot/bot
Length of output: 107
🏁 Script executed:
cat -n models/user.tsRepository: lnp2pBot/bot
Length of output: 2230
🏁 Script executed:
# Test what happens with new ObjectId(null)
python3 << 'EOF'
# Since we can't run MongoDB/Mongoose here, let's at least verify the MongoDB ObjectId behavior
# by checking if we have access to mongodb package
import subprocess
import sys
# Try to see if mongodb is available
try:
result = subprocess.run(['npm', 'list', 'mongodb'], cwd='.', capture_output=True, text=True)
print("NPM mongodb search:")
print(result.stdout)
print(result.stderr)
except Exception as e:
print(f"Error: {e}")
EOFRepository: lnp2pBot/bot
Length of output: 126
🏁 Script executed:
# Check package.json to see mongodb/mongoose versions
cat -n package.json | head -50Repository: lnp2pBot/bot
Length of output: 1711
🏁 Script executed:
# Search for any tests related to ObjectId or null handling
rg "ObjectId\(null\)" --type=tsRepository: lnp2pBot/bot
Length of output: 38
🏁 Script executed:
# Look for test cases that might reveal expected behavior
rg -A5 "new ObjectId" tests/ --type=ts | head -60Repository: lnp2pBot/bot
Length of output: 2630
🏁 Script executed:
# Check if there's any Mongoose usage documentation or comments
rg -B2 -A2 "auto.cast|auto-cast" --type=tsRepository: lnp2pBot/bot
Length of output: 38
🏁 Script executed:
# Look for how buyer_id and seller_id are used after being retrieved
rg -B2 -A2 "buyer_id\!|seller_id\!" bot/commands.ts | head -80Repository: lnp2pBot/bot
Length of output: 153
🏁 Script executed:
# Check if there's any validation before using these fields
rg "buyer_id|seller_id" bot/commands.ts -B1 -A1 | grep -E "if.*buyer_id|if.*seller_id" | head -20Repository: lnp2pBot/bot
Length of output: 505
🏁 Script executed:
# Look at the actual code patterns around line 151 where the review starts
sed -n '140,160p' bot/commands.tsRepository: lnp2pBot/bot
Length of output: 717
🏁 Script executed:
# Check if there are cases where buyer_id or seller_id could be null at the point of use
rg -B5 "new ObjectId\(order\.buyer_id" bot/commands.ts | head -40Repository: lnp2pBot/bot
Length of output: 1552
🏁 Script executed:
# Look for how order.buyer_id is checked before being used
rg -B10 "new ObjectId\(order\.buyer_id" bot/commands.ts | grep -A10 "order =" | head -50Repository: lnp2pBot/bot
Length of output: 1522
🏁 Script executed:
# Create a simple test to check ObjectId(null) behavior
python3 << 'EOF'
# Let's check what the mongodb/bson ObjectId library does with null
# First, let's look at package-lock.json or npm ls to see what's installed
import subprocess
import json
result = subprocess.run(['npm', 'ls', '--json'], capture_output=True, text=True)
try:
data = json.loads(result.stdout)
# Find mongodb or bson
for key in ['mongodb', 'bson']:
if key in data.get('dependencies', {}):
print(f"{key}: {data['dependencies'][key]}")
except:
print("Could not parse npm ls output")
# Also check package.json dependencies
with open('package.json') as f:
pkg = json.load(f)
deps = pkg.get('dependencies', {})
if 'mongoose' in deps:
print(f"mongoose: {deps['mongoose']}")
EOFRepository: lnp2pBot/bot
Length of output: 73
🏁 Script executed:
# Search for how ObjectId is imported and used - check if it's the mongodb ObjectId
rg "import.*ObjectId|from.*ObjectId" --type=ts | head -10Repository: lnp2pBot/bot
Length of output: 160
🏁 Script executed:
# Let's check what happens when we look at the actual usage patterns
# Find all instances where buyer_id or seller_id are used after being wrapped in ObjectId
rg -B3 -A3 'new ObjectId\(order\.(buyer_id|seller_id)' bot/commands.ts | head -100Repository: lnp2pBot/bot
Length of output: 3316
🏁 Script executed:
# Check if there are any defensive checks before using these fields
sed -n '230,245p' bot/commands.tsRepository: lnp2pBot/bot
Length of output: 617
🏁 Script executed:
# Look for similar patterns in other files to understand if this is intentional
rg -B2 'new ObjectId\(.*\.buyer_id' bot/validations.ts jobs/pending_payments.tsRepository: lnp2pBot/bot
Length of output: 267
🏁 Script executed:
# Check if there's error handling that catches ObjectId construction errors
rg -B2 -A2 "try.*new ObjectId|catch.*Error.*ObjectId" --type=tsRepository: lnp2pBot/bot
Length of output: 38
🏁 Script executed:
# Look for any comments or documentation about ObjectId usage in the codebase
rg -i "objectid.*null|null.*objectid" --type=ts -B1 -A1Repository: lnp2pBot/bot
Length of output: 38
🏁 Script executed:
# Check what version of mongodb driver is being used (indirectly through mongoose)
cat -n package-lock.json | grep -A5 -B5 '"bson"' | head -30Repository: lnp2pBot/bot
Length of output: 565
🏁 Script executed:
# Let's create a test to verify ObjectId behavior with null
python3 << 'EOF'
# We need to understand what ObjectId(null) actually does
# Let's check the mongoose/mongodb documentation behavior
# Since we're on Mongoose 9.5.0, let's look at what that version does
# by examining node_modules if available
import subprocess
import os
# Check if node_modules exists and has mongodb
result = subprocess.run(['find', 'node_modules', '-name', 'index.d.ts', '-path', '*bson*'],
capture_output=True, text=True, timeout=5)
if result.stdout:
print("Found BSON type definitions:")
files = result.stdout.strip().split('\n')[:3]
for f in files:
print(f)
# Try to find ObjectId constructor definition
result = subprocess.run(['grep', '-r', 'class ObjectId', 'node_modules/bson/', '--include=*.js', '--include=*.d.ts'],
capture_output=True, text=True, timeout=5)
if result.returncode == 0 and result.stdout:
print("\nObjectId definition found:")
print(result.stdout[:500])
EOFRepository: lnp2pBot/bot
Length of output: 38
🏁 Script executed:
# Check what the actual behavior is - look for error handling around ObjectId construction
rg "new ObjectId" bot/commands.ts -B5 -A5 | grep -E "try|catch|throw" -A2 -B2 | head -40Repository: lnp2pBot/bot
Length of output: 1672
🏁 Script executed:
# Look for any specific Mongoose 9 changes or notes in comments
rg -i "mongoose.*9|version.*9|auto.*cast" --type=ts --type=mdRepository: lnp2pBot/bot
Length of output: 38
🏁 Script executed:
# Verify if plain strings work by checking if there are any examples without ObjectId wrapping
rg "findOne.*{.*_id.*:" bot/validations.ts -A1 | grep -v "new ObjectId" | head -20Repository: lnp2pBot/bot
Length of output: 167
🏁 Script executed:
# Search for findOne patterns more carefully
rg "findOne.*_id" bot/validations.ts -A1 | head -30Repository: lnp2pBot/bot
Length of output: 589
🏁 Script executed:
# Check if there are any successful queries without ObjectId wrapping
rg "\.findOne\(" --type=ts -A1 | grep -v "new ObjectId" | head -40Repository: lnp2pBot/bot
Length of output: 2305
🏁 Script executed:
# Look at the test file to see how mocked queries handle this
rg "findOne" tests/bot/validation.spec.ts -B2 -A2 | head -60Repository: lnp2pBot/bot
Length of output: 1524
🏁 Script executed:
# This is crucial - check if there's a case where buyer_id or seller_id could be null at runtime
# Look for all places where an order is created/fetched, then used
rg -B5 "buyer_id.*null" models/order.tsRepository: lnp2pBot/bot
Length of output: 204
🏁 Script executed:
# Check the ln/subscribe_invoice.ts file more carefully since it doesn't wrap in ObjectId
sed -n '1,50p' ln/subscribe_invoice.tsRepository: lnp2pBot/bot
Length of output: 1536
🏁 Script executed:
# Look for what happens in subscribe_invoice with buyer_id/seller_id
sed -n '80,120p' ln/subscribe_invoice.tsRepository: lnp2pBot/bot
Length of output: 1535
🏁 Script executed:
# Find the critical part where buyer_id and seller_id are used without ObjectId wrapping
rg "buyer_id|seller_id" ln/subscribe_invoice.ts -B3 -A3Repository: lnp2pBot/bot
Length of output: 1109
🏁 Script executed:
# Check if ln/subscribe_invoice is using a different pattern - look for the actual queries
grep -n "User.findOne" ln/subscribe_invoice.ts -A1 -B1Repository: lnp2pBot/bot
Length of output: 729
🏁 Script executed:
# Let me see the full context around those User.findOne calls
sed -n '140,160p' ln/subscribe_invoice.tsRepository: lnp2pBot/bot
Length of output: 1144
🏁 Script executed:
# Check if there's any validation that ensures buyer_id/seller_id are not null before use
rg -B10 "User.findOne.*buyer_id" ln/subscribe_invoice.ts | grep -E "if.*buyer_id|if.*null|!=="Repository: lnp2pBot/bot
Length of output: 94
🏁 Script executed:
# Also check the pattern in bot/validations.ts to see if they check before wrapping
rg -B5 "new ObjectId\(.*Id\)" bot/validations.ts | head -40Repository: lnp2pBot/bot
Length of output: 578
Remove unnecessary new ObjectId() wrappings on string-typed ID fields — Mongoose 9 auto-casts them.
The codebase has two patterns: ln/subscribe_invoice.ts queries User.findOne({ _id: order.buyer_id }) directly (no ObjectId wrapper), while bot/commands.ts wraps the same fields with new ObjectId(order.buyer_id!). Both work identically in Mongoose 9, proving auto-casting.
Wrapping adds two problems:
- Silent miss on
nullvalues.order.buyer_idis typedstring | null. Whennullreachesnew ObjectId(null), it generates a random ObjectId instead of failing — converting a legitimate "not found" into a silent missed query. Direct strings let Mongoose's caster handle the null naturally. - Non-null assertion masks the type. The
!silences TypeScript but doesn't guarantee runtime safety. Ifbuyer_idisnullat runtime, the assertion is false.
Remove the new ObjectId() wrappings from the listed lines. If buyer_id or seller_id can legitimately be null, add explicit guards before queries:
if (!order.buyer_id) return;
const buyer = await User.findOne({ _id: order.buyer_id });
Applies to: 142, 151, 180, 233, 236, 238, 305, 315, 324, 326, 418, 422, 512, 521, 523, 651, 656, 742, 745, 835 in bot/commands.ts and similar patterns in bot/validations.ts, jobs/pending_payments.ts, bot/messages.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bot/commands.ts` at line 142, Remove the unnecessary new ObjectId(...)
wrappings around string-typed ID fields (e.g. places calling Order.findOne({
_id: new ObjectId(orderId) }) and User.findOne({ _id: new
ObjectId(order.buyer_id!) })) because Mongoose 9 auto-casts strings; instead
pass the raw string IDs to findOne. Replace occurrences that wrap
order.buyer_id, order.seller_id, orderId, etc., with the plain values and add
explicit null/undefined guards where the field is typed string | null (for
example, if (!order.buyer_id) return; const buyer = await User.findOne({ _id:
order.buyer_id });). Update all listed call sites in bot/commands.ts (and
similar patterns in bot/validations.ts, jobs/pending_payments.ts,
bot/messages.ts) to remove new ObjectId and drop non-null assertions so runtime
nulls are handled explicitly.
| } | ||
| if (user.banned) return await messages.bannedUserErrorMessage(ctx, user); | ||
| const order = await ordersActions.getOrder(ctx, user, orderId); | ||
| const order = await ordersActions.getOrder(ctx, user, new ObjectId(orderId) as any); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Mongoose 9.5, does Model.findOne({ _id: '<24-hex-string>' }) still auto-cast the string to ObjectId, or is explicit new mongoose.Types.ObjectId(...) required?
💡 Result:
Yes, in Mongoose 9.5, Model.findOne({ _id: '<24-hex-string>' }) still auto-casts the string to ObjectId. Explicit new mongoose.Types.ObjectId(...) is not required.
Citations:
- 1: https://mongoosejs.com/docs/api/model.html
- 2: https://mongoosejs.com/docs/queries.html
- 3: https://mongoosejs.com/docs/tutorials/query_casting.html
🏁 Script executed:
# First, check the bot/commands.ts file around lines 685 and 905
head -n 710 bot/commands.ts | tail -n +670 | cat -nRepository: lnp2pBot/bot
Length of output: 1809
🏁 Script executed:
# Check around line 905 too
head -n 920 bot/commands.ts | tail -n +890 | cat -nRepository: lnp2pBot/bot
Length of output: 1145
🏁 Script executed:
# Find the ordersActions.getOrder function definition
rg "getOrder\(" bot/ --type ts -A 10 | head -100Repository: lnp2pBot/bot
Length of output: 1138
🏁 Script executed:
# Find the ordersActions module and getOrder definition
fd "ordersActions" bot/ --type ts | head -5Repository: lnp2pBot/bot
Length of output: 226
🏁 Script executed:
# Search more broadly for getOrder definition
rg "const getOrder|export.*getOrder|getOrder\s*=" bot/ --type ts -B 2 -A 15Repository: lnp2pBot/bot
Length of output: 3605
🏁 Script executed:
# Get the complete getOrder implementation
rg "const getOrder = async" bot/ordersActions.ts -A 30Repository: lnp2pBot/bot
Length of output: 704
Remove new ObjectId() wrapping and type-casting that bypasses getOrder's validation.
getOrder expects an orderId: string parameter and validates it via ObjectId.isValid(orderId) before showing the user-facing notValidIdMessage on invalid input. Wrapping orderId in new ObjectId(...) as any causes the constructor to throw synchronously on invalid 24-hex strings, bypassing validation and user-facing error messaging. The outer catch (error) { logger.error(error); } swallows the exception silently—a UX regression. Mongoose 9.5 still auto-casts string filters to ObjectId for schema-typed _id fields, so pass the string directly.
🔧 Proposed fix
- const order = await ordersActions.getOrder(ctx, user, new ObjectId(orderId) as any);
+ const order = await ordersActions.getOrder(ctx, user, orderId);(apply at both line 685 and line 905)
📝 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.
| const order = await ordersActions.getOrder(ctx, user, new ObjectId(orderId) as any); | |
| const order = await ordersActions.getOrder(ctx, user, orderId); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bot/commands.ts` at line 685, The call currently constructs and casts an
ObjectId around orderId which bypasses ordersActions.getOrder's own validation;
update both occurrences (the call at the order retrieval near the earlier block
and the second similar call) to pass the raw string orderId directly to
ordersActions.getOrder so its ObjectId.isValid(orderId) check and user-facing
notValidIdMessage remain effective (remove new ObjectId(orderId) as any and pass
orderId: string to getOrder).
| throw new Error('counterPartyUser was not found'); | ||
|
|
||
| const updateOrder = await setCooperativeCancelFlag(order._id, initiator); | ||
| const updateOrder = await setCooperativeCancelFlag(new ObjectId(order._id) as any, initiator); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Drop the as any cast — setCooperativeCancelFlag accepts a string and order._id already serializes cleanly.
setCooperativeCancelFlag(orderId: string, ...) is typed for a string, but the call passes new ObjectId(order._id) as any — order._id is already a mongoose.Types.ObjectId, so the wrapping is a no-op other than forcing an unsafe cast. Use order._id.toString() and let the function's _id: new ObjectId(orderId) (line 46) do the conversion, or better, also drop that wrapping and rely on Mongoose's auto-cast.
🔧 Proposed fix
- const updateOrder = await setCooperativeCancelFlag(new ObjectId(order._id) as any, initiator);
+ const updateOrder = await setCooperativeCancelFlag(order._id.toString(), initiator);📝 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.
| const updateOrder = await setCooperativeCancelFlag(new ObjectId(order._id) as any, initiator); | |
| const updateOrder = await setCooperativeCancelFlag(order._id.toString(), initiator); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bot/commands.ts` at line 751, The call to setCooperativeCancelFlag currently
wraps order._id with new ObjectId(...) and casts to any; remove the unsafe cast
and pass a string instead (e.g., use order._id.toString() or pass order._id
directly if already a string) so the call becomes
setCooperativeCancelFlag(order._id.toString(), initiator); update callers to
stop using "as any" and, optionally, remove the redundant new ObjectId(orderId)
conversion inside setCooperativeCancelFlag if you want to rely on Mongoose
auto-casting.
| const isScheduled = await PendingPayment.findOne({ | ||
| community_id: communityId, | ||
| attempts: { $lt: process.env.PAYMENT_ATTEMPTS }, | ||
| attempts: { $lt: Number(process.env.PAYMENT_ATTEMPTS) }, | ||
| paid: false, | ||
| }); |
There was a problem hiding this comment.
Number(process.env.PAYMENT_ATTEMPTS) can become NaN and silently disable the duplicate-payment guard.
If PAYMENT_ATTEMPTS is unset/invalid, Number(...) is NaN and attempts: { $lt: NaN } matches no documents, so isScheduled will be null and the "invoice already being paid" guard is bypassed — allowing parallel withdrawal attempts.
This same logic now exists in jobs/pending_payments.ts as getPaymentAttempts() (with a || 2 fallback). Consider exporting that helper and reusing it here (and in bot/modules/community/scenes.ts:947-951, bot/start.ts:1018-1021) for a single source of truth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bot/modules/community/messages.ts` around lines 159 - 163, The guard using
Number(process.env.PAYMENT_ATTEMPTS) can become NaN and bypass the
duplicate-payment check; replace that ad-hoc conversion by importing and using
the shared getPaymentAttempts() helper from jobs/pending_payments.ts (export it
if not already exported) and use it in the PendingPayment.findOne query (the
same change should be applied to bot/modules/community/scenes.ts around the
withdrawal logic and bot/start.ts where PAYMENT_ATTEMPTS is used) so attempts
uses a validated numeric fallback (e.g., || 2) instead of
Number(process.env.PAYMENT_ATTEMPTS).
| order_id: order._id, | ||
| attempts: { $lt: process.env.PAYMENT_ATTEMPTS }, | ||
| order_id: order._id.toString(), | ||
| attempts: { $lt: Number(process.env.PAYMENT_ATTEMPTS) }, |
There was a problem hiding this comment.
Number(process.env.PAYMENT_ATTEMPTS) silently becomes NaN if the env var is missing.
$lt: NaN will never match any document, which would defeat this duplicate‑payment guard and let a second PendingPayment slip through. Consider validating the variable at startup (or here) and failing fast, mirroring the existing pattern used for HOLD_INVOICE_EXPIRATION_WINDOW above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bot/scenes.ts` at line 149, The filter uses
Number(process.env.PAYMENT_ATTEMPTS) which can become NaN and make the $lt check
always fail; define and validate a PAYMENT_ATTEMPTS constant at startup (like
the existing HOLD_INVOICE_EXPIRATION_WINDOW pattern) by parsing
process.env.PAYMENT_ATTEMPTS (e.g., parseInt) and throwing/failing fast if it is
missing or NaN, then replace Number(process.env.PAYMENT_ATTEMPTS) in the
attempts: { $lt: ... } expression with that validated PAYMENT_ATTEMPTS constant
so the duplicate‑payment guard works reliably.
| import mongoose from 'mongoose'; | ||
| const { ObjectId } = mongoose.Types; |
There was a problem hiding this comment.
ObjectId is imported but never used — remove the destructure (and the mongoose import if unused).
ESLint flags this; the file only does _id.toString() and string equality—ObjectId is never referenced. As per coding guidelines: "Follow ESLint Standard plus TypeScript rules and address warnings instead of disabling them".
🧹 Proposed fix
-import mongoose from 'mongoose';
-const { ObjectId } = mongoose.Types;📝 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.
| import mongoose from 'mongoose'; | |
| const { ObjectId } = mongoose.Types; | |
| import mongoose from 'mongoose'; |
🧰 Tools
🪛 ESLint
[error] 7-7: 'ObjectId' is assigned a value but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 7-7: 'ObjectId' is assigned a value but never used.
(no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bot/start.ts` around lines 6 - 7, Remove the unused destructure "ObjectId"
(and the "mongoose" import if it's only present to obtain ObjectId) from this
module: locate the import line that reads "import mongoose from 'mongoose';
const { ObjectId } = mongoose.Types;" and delete the ObjectId destructure (and
the whole mongoose import if no other symbol from mongoose is referenced
elsewhere in the file); ensure any existing code that uses _id.toString() or
string comparisons continues to work without ObjectId, adjust imports/types if
necessary, and run ESLint to confirm the warning is resolved.
| }); | ||
| for (const pending of pendingPayments) { | ||
| const order = await Order.findOne({ _id: pending.order_id }); | ||
| const order = await Order.findOne({ _id: new ObjectId(pending.order_id) }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Mongoose 9.5 still auto-cast string filter values to ObjectId for schema-typed _id fields, or does it require explicit new ObjectId(...) wrapping?
💡 Result:
Yes, Mongoose 9.5 still auto-casts string filter values to ObjectId for schema-typed _id fields. It does not require explicit new ObjectId(...) wrapping.
Citations:
- 1: https://mongoosejs.com/docs/tutorials/query_casting.html
- 2: https://mongoosejs.com/docs/api/model.html
- 3: https://mongoosejs.com/docs/6.x/docs/tutorials/query_casting.html
🏁 Script executed:
# Locate and examine jobs/pending_payments.ts
fd -t f "pending_payments.ts"Repository: lnp2pBot/bot
Length of output: 81
🏁 Script executed:
# Find the Order model/interface definition
fd -t f -e ts | xargs rg -l "interface IOrder|class Order" | head -20Repository: lnp2pBot/bot
Length of output: 92
🏁 Script executed:
# Check for the exact lines mentioned in the review
fd -t f "pending_payments.ts" -exec wc -l {} \;Repository: lnp2pBot/bot
Length of output: 243
🏁 Script executed:
cat -n jobs/pending_payments.ts | head -100Repository: lnp2pBot/bot
Length of output: 4569
🏁 Script executed:
cat -n models/order.ts | head -150Repository: lnp2pBot/bot
Length of output: 6007
🏁 Script executed:
cat -n jobs/pending_payments.ts | sed -n '190,220p'Repository: lnp2pBot/bot
Length of output: 1553
🏁 Script executed:
fd -t f -e ts "pending_payment" models/Repository: lnp2pBot/bot
Length of output: 82
🏁 Script executed:
fd -t f -e ts "user\\.ts" models/Repository: lnp2pBot/bot
Length of output: 71
🏁 Script executed:
cat -n models/pending_payment.tsRepository: lnp2pBot/bot
Length of output: 1741
🏁 Script executed:
cat -n models/user.ts | head -100Repository: lnp2pBot/bot
Length of output: 2230
Remove unnecessary new ObjectId() wrapping and explicit null checks for nullable fields on lines 63 and 87.
Mongoose 9.5 still auto-casts string values to ObjectId in filters without explicit wrapping, so new ObjectId(...) is unnecessary throughout. More critically, lines 63 and 87 use non-null assertions (buyer_id! and seller_id!) on fields typed as string | null in the IOrder interface. At runtime, if either is null, new ObjectId(null) generates a random ObjectId instead of failing, leading to a silent match against the wrong user. Replace the assertions with explicit null checks:
if (!order.buyer_id) throw Error('buyer_id is required');
const buyerUser = await User.findOne({ _id: order.buyer_id });Or simplify to:
const buyerUser = await User.findOne({ _id: order.buyer_id });
if (buyerUser === null) throw Error('buyerUser was not found in DB');Apply the same pattern to line 87 for seller_id.
Also applies to: 28-28, 197-197, 209-209 (remove unnecessary wrapping)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jobs/pending_payments.ts` at line 28, Remove unnecessary use of new
ObjectId(...) when querying (e.g., Order.findOne({ _id: new
ObjectId(pending.order_id) }) and other similar queries) and instead pass the
string directly so Mongoose can auto-cast; additionally, replace non-null
assertions on nullable fields buyer_id and seller_id with explicit null checks
and proper error handling before querying (e.g., verify order.buyer_id and
order.seller_id are present and throw or return a clear error if missing), then
call User.findOne({ _id: order.buyer_id }) / User.findOne({ _id: order.seller_id
}) and handle the case where the returned buyerUser or sellerUser is null.
| "invoices": "2.0.6", | ||
| "lightning": "10.25.0", | ||
| "mongoose": "^8.17.1", | ||
| "mongoose": "^9.5.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the minimum supported Node.js version for mongoose 9.x?
💡 Result:
The minimum supported Node.js version for Mongoose 9.x is 18.
Citations:
🏁 Script executed:
# Check the latest Mongoose 9.x versions available on npm
npm view mongoose@9 versions --json 2>/dev/null | tail -20Repository: lnp2pBot/bot
Length of output: 295
🏁 Script executed:
# Also check the specific version metadata to see if 9.5.0 exists and Node requirements
npm view mongoose@9.5.0 2>/dev/null || echo "9.5.0 does not exist"
npm view mongoose@9.4.1 engines 2>/dev/null
npm view mongoose@9.0.0 engines 2>/dev/nullRepository: lnp2pBot/bot
Length of output: 953
🏁 Script executed:
# Check the current package.json to see engines declaration
head -50 package.json | grep -A 2 '"engines"'Repository: lnp2pBot/bot
Length of output: 99
Mongoose 9.5.0 has incompatible Node engine requirements with the project.
One blocking problem:
Mongoose 9.5.0 requires Node.js ≥ 20.19.0 but engines.node here declares >=18.0.0. Bump the engine constraint to >=20.19.0 to resolve the incompatibility and update the Node version in your CI pipeline accordingly.
Note: Mongoose 9.0.0 introduced several breaking changes (driver upgrade, removed deprecated options/types, schema/Document casting changes). Please review the migration guide and add regression testing before merging — this is more than a simple version bump.
🧰 Tools
🪛 GitHub Actions: Auto Check Lint
[warning] npm EBADENGINE: Unsupported engine(s) detected (node v18.20.8 current, required >=20.x for bson/ecpair/mongodb/mongoose and others).
[warning] npm audit: 25 vulnerabilities found (4 low, 7 moderate, 14 high).
[warning] npm deprecated dependencies detected (e.g., rimraf@3.0.2, inflight@1.0.6, glob@7.2.0, crypto@1.0.1, eslint@8.57.1, etc.).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 31, package.json currently depends on "mongoose":
"^9.5.0" which requires Node.js >=20.19.0 but the package.json "engines.node"
declares >=18.0.0; update the "engines.node" constraint to ">=20.19.0" and align
your CI workflow (node version in CI config) to use >=20.19.0, then run/author
regression tests around code paths using mongoose (and review mongoose 9.x
migration notes) to catch breaking changes before merging; look for the
"mongoose" dependency and the "engines.node" field to apply these changes.
This is because CI uses latest Ubuntu LTS, which right now is 24.04, which has NodeJS 18.x. IMO this PR shouldn't merged before May. This way, you can upgrade CI first to 26.04 when it's released (in theory it will be very soon, in late April), which will bring NodeJS 22.x. |
2 participants
Summary by CodeRabbit
Bug Fixes
Chores