feat: upgrade mongoose from 6.13.6 to 9.6.2#805
Open
ToRyVand wants to merge 5 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR consistently converts MongoDB ObjectId usage to string IDs via ChangesMongoDB ID Stringification & Type System Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bot/commands.ts (1)
771-776:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse strict equality and consistent ObjectId stringification.
Two issues in the cooperative cancel flow:
- Line 771 uses
String(user._id)while the rest of the PR uses.toString()(e.g., line 692). This inconsistency reduces code maintainability.- Line 776 uses loose equality
==instead of strict===. Loose equality can cause unexpected type coercion bugs.🔧 Proposed fix
- updateOrder.canceled_by = String(user._id); + updateOrder.canceled_by = user._id.toString(); await updateOrder.save(); let seller = initiatorUser; let i18nCtxSeller = ctx.i18n; - if (order.seller_id == counterPartyUser._id.toString()) { + if (order.seller_id === counterPartyUser._id.toString()) { seller = counterPartyUser;As per coding guidelines, follow ESLint Standard plus TypeScript rules and address warnings instead of disabling them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bot/commands.ts` around lines 771 - 776, Replace the inconsistent stringification and loose equality: set updateOrder.canceled_by using user._id.toString() (not String(user._id)) and change the comparison order.seller_id == counterPartyUser._id.toString() to a strict equality using === (order.seller_id === counterPartyUser._id.toString()); update the cooperative cancel flow around updateOrder, user._id, order.seller_id and counterPartyUser._id accordingly so all ObjectId comparisons use .toString() and ===.bot/ordersActions.ts (1)
315-343:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftApply
.toString()touser._idin thegetOrdersquery for consistency and correctness.The
getOrderfunction correctly usesuser._id.toString()when comparing against string-type fields (buyer_idandseller_id), butgetOrdersuses rawuser._id. Since the Order schema stores these fields as strings anduser._idis a MongoDB ObjectId, the query at line 320 should be updated:$or: [{ buyer_id: user._id.toString() }, { seller_id: user._id.toString() }],Without this fix, the query may fail to match orders correctly due to the ObjectId/string type mismatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bot/ordersActions.ts` around lines 315 - 343, In getOrders, the query compares Order string fields buyer_id and seller_id against a MongoDB ObjectId (user._id); change the $or clause inside the where construction to use user._id.toString() for both buyer_id and seller_id so the query matches string IDs correctly (update the $or: [{ buyer_id: user._id }, { seller_id: user._id }] to use .toString() in the getOrders function).
🧹 Nitpick comments (6)
tsconfig.json (1)
4-4: ⚡ Quick winScope
skipLibCheckas a temporary workaround instead of a global default.Line 4 applies this to every TS compile path, which can mask declaration-file breakages from other dependencies. Prefer isolating it to the specific build/check config that needs it (or tracking explicit removal criteria tied to the upstream Mongoose/TS typing issue).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tsconfig.json` at line 4, Currently "skipLibCheck" is set globally which can hide declaration-file issues; limit this workaround to the specific build/check configuration that needs it by removing "skipLibCheck" from the shared tsconfig and adding it only to a targeted tsconfig used for the failing build (e.g., a dedicated tsconfig for that package or a temporary tsconfig used by the CI step), update your build/tsc invocation to reference the targeted tsconfig, and add a TODO comment and an issue/PR link to the Mongoose/TS typing tracker so the scoped override can be removed once upstream typings are fixed.util/index.ts (1)
40-40: ⚡ Quick winUse strict equality (
===) instead of loose equality (==).Both comparison statements use loose equality (
==) instead of strict equality (===). Strict equality is the TypeScript/JavaScript standard and prevents unexpected type coercion behavior.♻️ Proposed fix
Line 40:
- return user._id.toString() == order.creator_id; + return user._id.toString() === order.creator_id;Line 457:
- return community.solvers.some(solver => solver.id == user._id.toString()); + return community.solvers.some(solver => solver.id === user._id.toString());As per coding guidelines, code should follow ESLint Standard plus TypeScript rules.
Also applies to: 457-457
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@util/index.ts` at line 40, Change the loose equality checks to strict equality: replace the comparison in util/index.ts that currently returns user._id.toString() == order.creator_id with a strict ===, and make the same change for the other occurrence mentioned (around the second instance). Ensure both comparisons use === so types are compared without coercion (refer to the user._id.toString() and order.creator_id comparisons).bot/modules/dispute/commands.ts (2)
38-38: ⚡ Quick winUse strict equality operator.
Replace
==with===for strict equality comparison. As per coding guidelines, ESLint Standard requires strict equality operators.Suggested fix
- if (user._id.toString() == order.buyer_id) initiator = 'buyer'; + if (user._id.toString() === order.buyer_id) initiator = 'buyer';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bot/modules/dispute/commands.ts` at line 38, Change the loose equality to strict equality in the conditional that sets the dispute initiator: replace the comparison using user._id.toString() == order.buyer_id with a strict comparison (use ===) so the if statement that assigns initiator = 'buyer' uses user._id.toString() === order.buyer_id; leave the rest of the logic unchanged.
167-173: ⚡ Quick winUse strict equality operators.
Replace loose equality operators (
!=,==) with strict operators (!==,===) to comply with ESLint Standard coding guidelines.Suggested fixes
- if (dispute && dispute.solver_id != admin._id.toString()) { + if (dispute && dispute.solver_id !== admin._id.toString()) { return await globalMessages.notAuthorized(ctx); } } - if (user._id.toString() == dispute.buyer_id) dispute.buyer_id = null; - if (user._id.toString() == dispute.seller_id) dispute.seller_id = null; + if (user._id.toString() === dispute.buyer_id) dispute.buyer_id = null; + if (user._id.toString() === dispute.seller_id) dispute.seller_id = null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bot/modules/dispute/commands.ts` around lines 167 - 173, The code is using loose equality operators in the dispute permission and ID-clearing checks; update the comparisons in the block that calls globalMessages.notAuthorized(ctx) and the subsequent buyer/seller ID nulling so they use strict operators: replace != with !== when comparing dispute.solver_id to admin._id.toString(), and replace == with === when comparing user._id.toString() to dispute.buyer_id and dispute.seller_id (keep the existing toString() calls so types match). Ensure the checks still short-circuit correctly around the dispute variable.bot/modules/community/scenes.ts (1)
396-400: ⚡ Quick winInvestigate the type assertion bypass.
The double type assertion
as unknown as IUsernameIdbypasses TypeScript's type checking and may hide a type mismatch. This pattern suggests the object structure doesn't naturally matchIUsernameId.Consider either:
- Updating the
IUsernameIdtype definition to match the actual structure- Adjusting the object to properly conform to
IUsernameId- If the cast is intentional, adding a comment explaining why it's necessary
The same issue appears at lines 748-750 in the update solvers wizard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bot/modules/community/scenes.ts` around lines 396 - 400, The double-cast bypass on the object pushed into solvers (the push that uses "as unknown as IUsernameId") indicates the object shape doesn't match the IUsernameId interface; fix by either updating IUsernameId to include the fields being used (id: string, username: string) or by constructing an object that exactly matches IUsernameId (e.g., map/convert user._id and user.username to the expected property names/types) in the solvers.push site and in the analogous push inside the update solvers wizard; if you intentionally need the cast, replace the double-cast with a single explicit cast and add a clarifying comment explaining why the runtime shape differs from the type.bot/start.ts (1)
504-504: ⚡ Quick winInconsistent ID field usage:
.idvs._id.toString().Line 504 assigns
order.canceled_by = ctx.user.id, while lines 355 and 421 usectx.admin._id.toString()for similar audit field assignments. Both approaches produce string values, but mixing them reduces consistency. Consider usingctx.user._id.toString()to match the pattern used elsewhere in this file.♻️ Align with established pattern
- order.canceled_by = ctx.user.id; + order.canceled_by = ctx.user._id.toString();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bot/start.ts` at line 504, The assignment to order.canceled_by uses ctx.user.id, which is inconsistent with other audit assignments that use ctx.admin._id.toString(); update the assignment to use the same pattern by setting order.canceled_by = ctx.user._id.toString() so all audit fields are stored as strings consistently (match the style used for ctx.admin._id.toString()).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bot/modules/orders/scenes.ts`:
- Around line 307-313: The code assumes ctx.message always exists before
deleting and parsing: guard for ctx.message first to avoid exceptions from
ctx.deleteMessage(); change the flow in the non-callback branch so you first
check if ctx.message is present (e.g., if (!ctx.message) { set
ctx.wizard.state.error = ctx.i18n.t('some_key_for_missing_message'); await
ctx.wizard.state.updateUI(); return; }), then read Number(ctx.message.text) into
rawInput and call await ctx.deleteMessage(); keep the existing isNaN(rawInput)
check and error handling (ctx.wizard.state.error and ctx.wizard.state.updateUI)
unchanged; reference: rawInput, ctx.deleteMessage, ctx.message,
ctx.wizard.state.updateUI.
In `@models/order.ts`:
- Line 4: IOrder no longer exposes _id, so replace all order._id usages with
order.id across the listed files (jobs/pending_payments.ts,
delete_published_orders.ts, cancel_orders.ts, check_hold_invoice_expired.ts;
bot/start.ts, scenes.ts, commands.ts, messages.ts, ordersActions.ts,
modules/dispute/messages.ts, modules/dispute/commands.ts;
ln/subscribe_invoice.ts, pay_request.ts), updating logging/template strings to
use order.id and fixing TypeScript errors; for database queries that currently
use { _id: order._id } either change them to query by the id field (e.g. { id:
order.id }) if your DB schema supports it or convert order.id to the proper
ObjectId type before querying (e.g. construct ObjectId(order.id)) so the query
syntax matches your DB driver. Ensure imports/types compile after the
replacements and run the test/type-check to catch remaining references.
In `@models/user.ts`:
- Line 9: Several places still assign Mongo ObjectId via user._id even though
UserDocument now exposes id: string; update assignments in bot/ordersActions.ts,
bot/modules/dispute/commands.ts, bot/start.ts, and
bot/modules/community/scenes.ts to use the string id (user.id) or call
user._id.toString() consistently; locate the specific assignment sites (e.g.,
where an order, dispute, session, or community record sets a user field from
user._id) and replace with user.id (preferred) or user._id.toString(), ensuring
all persisted fields and query parameters are strings to match the updated
UserDocument contract.
In `@package.json`:
- Line 31: The package.json declares "mongoose": "^9.6.2" but the "engines.node"
field is still ">=18.0.0", which is incompatible with Mongoose 9's Node
requirement (>=20.19.0); update the engines.node constraint in package.json to
at least ">=20.19.0" (or raise it to a suitable range like ">=20.19.0 <22" if
you want to bound major versions) so that the "mongoose" dependency and the
"engines.node" entry are consistent, or alternatively downgrade the "mongoose"
entry to a version compatible with Node 18 if you must keep Node 18 support.
In `@tests/bot/validation.spec.ts`:
- Around line 150-155: The env stubs in tests (e.g., the sandbox.stub(process,
'env').value({...}) block in validation.spec.ts) use numeric values but Node's
process.env values are always strings; change all stubbed env values
(MIN_PAYMENT_AMT, MAX_PAYMENT_AMT, NODE_ENV, INVOICE_EXPIRATION_WINDOW, etc.) to
string literals so the code paths that call Number(process.env.X) and similar
parsing logic are exercised; apply the same stringification to the other stub
blocks mentioned in the comment (lines around 166-170, 186-190, 202-206,
290-294, 305-309, 319-323, 335-339, 1254-1257, 1269-1272, 1290-1293, 1304-1307,
1318-1321).
---
Outside diff comments:
In `@bot/commands.ts`:
- Around line 771-776: Replace the inconsistent stringification and loose
equality: set updateOrder.canceled_by using user._id.toString() (not
String(user._id)) and change the comparison order.seller_id ==
counterPartyUser._id.toString() to a strict equality using === (order.seller_id
=== counterPartyUser._id.toString()); update the cooperative cancel flow around
updateOrder, user._id, order.seller_id and counterPartyUser._id accordingly so
all ObjectId comparisons use .toString() and ===.
In `@bot/ordersActions.ts`:
- Around line 315-343: In getOrders, the query compares Order string fields
buyer_id and seller_id against a MongoDB ObjectId (user._id); change the $or
clause inside the where construction to use user._id.toString() for both
buyer_id and seller_id so the query matches string IDs correctly (update the
$or: [{ buyer_id: user._id }, { seller_id: user._id }] to use .toString() in the
getOrders function).
---
Nitpick comments:
In `@bot/modules/community/scenes.ts`:
- Around line 396-400: The double-cast bypass on the object pushed into solvers
(the push that uses "as unknown as IUsernameId") indicates the object shape
doesn't match the IUsernameId interface; fix by either updating IUsernameId to
include the fields being used (id: string, username: string) or by constructing
an object that exactly matches IUsernameId (e.g., map/convert user._id and
user.username to the expected property names/types) in the solvers.push site and
in the analogous push inside the update solvers wizard; if you intentionally
need the cast, replace the double-cast with a single explicit cast and add a
clarifying comment explaining why the runtime shape differs from the type.
In `@bot/modules/dispute/commands.ts`:
- Line 38: Change the loose equality to strict equality in the conditional that
sets the dispute initiator: replace the comparison using user._id.toString() ==
order.buyer_id with a strict comparison (use ===) so the if statement that
assigns initiator = 'buyer' uses user._id.toString() === order.buyer_id; leave
the rest of the logic unchanged.
- Around line 167-173: The code is using loose equality operators in the dispute
permission and ID-clearing checks; update the comparisons in the block that
calls globalMessages.notAuthorized(ctx) and the subsequent buyer/seller ID
nulling so they use strict operators: replace != with !== when comparing
dispute.solver_id to admin._id.toString(), and replace == with === when
comparing user._id.toString() to dispute.buyer_id and dispute.seller_id (keep
the existing toString() calls so types match). Ensure the checks still
short-circuit correctly around the dispute variable.
In `@bot/start.ts`:
- Line 504: The assignment to order.canceled_by uses ctx.user.id, which is
inconsistent with other audit assignments that use ctx.admin._id.toString();
update the assignment to use the same pattern by setting order.canceled_by =
ctx.user._id.toString() so all audit fields are stored as strings consistently
(match the style used for ctx.admin._id.toString()).
In `@tsconfig.json`:
- Line 4: Currently "skipLibCheck" is set globally which can hide
declaration-file issues; limit this workaround to the specific build/check
configuration that needs it by removing "skipLibCheck" from the shared tsconfig
and adding it only to a targeted tsconfig used for the failing build (e.g., a
dedicated tsconfig for that package or a temporary tsconfig used by the CI
step), update your build/tsc invocation to reference the targeted tsconfig, and
add a TODO comment and an issue/PR link to the Mongoose/TS typing tracker so the
scoped override can be removed once upstream typings are fixed.
In `@util/index.ts`:
- Line 40: Change the loose equality checks to strict equality: replace the
comparison in util/index.ts that currently returns user._id.toString() ==
order.creator_id with a strict ===, and make the same change for the other
occurrence mentioned (around the second instance). Ensure both comparisons use
=== so types are compared without coercion (refer to the user._id.toString() and
order.creator_id comparisons).
🪄 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: 581aabe2-57b1-4650-9e71-b41519d5349a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
.env-samplebot/commands.tsbot/messages.tsbot/modules/community/commands.tsbot/modules/community/messages.tsbot/modules/community/scenes.tsbot/modules/dispute/actions.tsbot/modules/dispute/commands.tsbot/modules/orders/commands.tsbot/modules/orders/scenes.tsbot/modules/orders/takeOrder.tsbot/ordersActions.tsbot/scenes.tsbot/start.tsbot/validations.tsjobs/pending_payments.tslocales/de.yamllocales/en.yamllocales/es.yamllocales/fa.yamllocales/fr.yamllocales/it.yamllocales/ko.yamllocales/pt.yamllocales/ru.yamllocales/uk.yamlmodels/community.tsmodels/order.tsmodels/user.tspackage.jsontests/bot/validation.spec.tstsconfig.jsonutil/communityHelper.tsutil/index.ts
|
|
||
| export interface IOrder extends Document { | ||
| _id: string; | ||
| id: string; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find references to order._id that may need updating after interface change
# Search for patterns like order._id, o._id where the variable is likely an IOrder
rg -n --type=ts -C3 '\b(order|o)\._id\b' \
-g '!models/order.ts' \
-g '!node_modules/**'Repository: lnp2pBot/bot
Length of output: 31737
Migrate all order._id references to order.id throughout the codebase—this is now a type error.
The interface change removes _id from IOrder, creating ~80 TypeScript errors across 12+ files. Files requiring updates:
- jobs/: pending_payments.ts (4), delete_published_orders.ts (1), cancel_orders.ts (2), check_hold_invoice_expired.ts (3)
- bot/: start.ts (6), scenes.ts (3), commands.ts (10+), messages.ts (40+), ordersActions.ts (1), modules/dispute/messages.ts (4), modules/dispute/commands.ts (1)
- ln/: subscribe_invoice.ts (5), pay_request.ts (5)
Most occurrences are in logging and string templates where order.id (the new string getter) is appropriate. For database queries currently using { _id: order._id }, verify the query syntax still works or switch to querying by the id field where applicable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@models/order.ts` at line 4, IOrder no longer exposes _id, so replace all
order._id usages with order.id across the listed files
(jobs/pending_payments.ts, delete_published_orders.ts, cancel_orders.ts,
check_hold_invoice_expired.ts; bot/start.ts, scenes.ts, commands.ts,
messages.ts, ordersActions.ts, modules/dispute/messages.ts,
modules/dispute/commands.ts; ln/subscribe_invoice.ts, pay_request.ts), updating
logging/template strings to use order.id and fixing TypeScript errors; for
database queries that currently use { _id: order._id } either change them to
query by the id field (e.g. { id: order.id }) if your DB schema supports it or
convert order.id to the proper ObjectId type before querying (e.g. construct
ObjectId(order.id)) so the query syntax matches your DB driver. Ensure
imports/types compile after the replacements and run the test/type-check to
catch remaining references.
Comment on lines
+150
to
+155
| sandbox.stub(process, 'env').value({ | ||
| MIN_PAYMENT_AMT: 100, | ||
| MAX_PAYMENT_AMT: 5000, | ||
| NODE_ENV: 'production', | ||
| INVOICE_EXPIRATION_WINDOW: 3600000, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify numeric env literals inside process.env stubs in this spec file.
# Expected: matches indicate non-string env fixtures that should be stringified.
rg -n "sandbox\\.stub\\(process, 'env'\\)\\.value\\(" tests/bot/validation.spec.ts -A8
rg -n "(_PAYMENT_AMT|INVOICE_EXPIRATION_WINDOW):\\s*[0-9]+" tests/bot/validation.spec.tsRepository: lnp2pBot/bot
Length of output: 6760
🏁 Script executed:
# Find and examine the validation function implementations
find . -type f \( -name "*.ts" -o -name "*.js" \) ! -path "*/node_modules/*" ! -path "*/.git/*" | head -20
# Look for validateSellOrder and validateBuyOrder function definitions
rg -n "export.*validateSellOrder|export.*validateBuyOrder|function validateSellOrder|function validateBuyOrder" --type ts --type js -A 10Repository: lnp2pBot/bot
Length of output: 650
🏁 Script executed:
# Search for how MIN_PAYMENT_AMT, MAX_PAYMENT_AMT are used in the codebase
rg -n "MIN_PAYMENT_AMT|MAX_PAYMENT_AMT" --type ts --type js -B 2 -A 2 | head -80Repository: lnp2pBot/bot
Length of output: 4859
🏁 Script executed:
# Find the validation module/file
fd -t f "validation" --type-list all | grep -E "\.(ts|js)$"
# Look for imports of validateSellOrder in the test file
rg -n "from.*validation|import.*validateSellOrder" tests/bot/validation.spec.ts | head -5Repository: lnp2pBot/bot
Length of output: 309
🏁 Script executed:
# Search for how process.env is accessed in validation functions
rg -n "process\.env\.(MIN_PAYMENT_AMT|MAX_PAYMENT_AMT|INVOICE_EXPIRATION_WINDOW)" --type ts --type js -B 3 -A 3Repository: lnp2pBot/bot
Length of output: 4947
Stub environment variables as strings to match Node.js runtime behavior.
In production, process.env values are always strings. The validation code explicitly converts these values using Number(process.env.MIN_PAYMENT_AMT) and similar patterns, meaning tests should stub with string values to exercise the actual parsing path and catch potential edge cases (whitespace, malformed values, etc.).
Suggested adjustment (apply similarly to all new env stubs)
sandbox.stub(process, 'env').value({
- MIN_PAYMENT_AMT: 100,
- MAX_PAYMENT_AMT: 5000,
+ MIN_PAYMENT_AMT: '100',
+ MAX_PAYMENT_AMT: '5000',
NODE_ENV: 'production',
- INVOICE_EXPIRATION_WINDOW: 3600000,
+ INVOICE_EXPIRATION_WINDOW: '3600000',
});Also applies to: 166-170, 186-190, 202-206, 290-294, 305-309, 319-323, 335-339, 1254-1257, 1269-1272, 1290-1293, 1304-1307, 1318-1321
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/bot/validation.spec.ts` around lines 150 - 155, The env stubs in tests
(e.g., the sandbox.stub(process, 'env').value({...}) block in
validation.spec.ts) use numeric values but Node's process.env values are always
strings; change all stubbed env values (MIN_PAYMENT_AMT, MAX_PAYMENT_AMT,
NODE_ENV, INVOICE_EXPIRATION_WINDOW, etc.) to string literals so the code paths
that call Number(process.env.X) and similar parsing logic are exercised; apply
the same stringification to the other stub blocks mentioned in the comment
(lines around 166-170, 186-190, 202-206, 290-294, 305-309, 319-323, 335-339,
1254-1257, 1269-1272, 1290-1293, 1304-1307, 1318-1321).
ToRyVand
force-pushed
the
feat/issue-655-mongoose-upgrade
branch
from
88dc98e to
b42f5c4
Compare
knocte
reviewed
May 14, 2026
| "version": "0.15.0", | ||
| "engines": { | ||
| "node": ">=18.0.0" | ||
| "node": ">=20.19.0" |
Collaborator
There was a problem hiding this comment.
@ToRyVand this should be coupled with upgrading ".github/workflows/integrate.yml" workflow to replace image: 'ubuntu:24.04' with image: 'ubuntu:26.04' because Ubuntu24.04 still has NodeJS 18.x
Collaborator
|
BTW, run prettier to fix formatter CI step |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/integrate.yaml (1)
16-16: Consider separating infrastructure changes from feature changes.This PR is focused on upgrading mongoose (6.13.6 → 9.6.2), but these CI infrastructure changes (Ubuntu 24.04 → 26.04, jammy → noble) are unrelated to that objective. While not necessarily incorrect, separating infrastructure updates into a dedicated PR improves:
- Change tracking and rollback capability
- Review focus and context
- Bisect-ability when investigating issues
Also applies to: 43-43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/integrate.yaml at line 16, The CI change that updates the workflow image from the previous Ubuntu (the line "image: 'ubuntu:26.04'") and related label changes (jammy → noble) are unrelated to the mongoose upgrade; revert the workflow modification in the integrate.yaml (restore the prior image value e.g. the earlier ubuntu version and original distro label) so this PR only contains the mongoose bump, and open a separate PR that contains the infrastructure changes (the image: 'ubuntu:26.04' update and any distro name changes) with its own description and testing notes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/integrate.yaml:
- Line 16: The workflow uses image: 'ubuntu:26.04' while the MongoDB apt
repository is set to the 'noble' (Ubuntu 24.04) distribution codename; either
change the Docker image to ubuntu:24.04 to match the 'noble' repo or update the
MongoDB repo configuration to the correct codename that matches Ubuntu 26.04
(after verifying MongoDB supports 26.04) — locate the image line "image:
'ubuntu:26.04'" and the apt repo entry that references "noble" and make them
consistent (swap the image to ubuntu:24.04 or replace 'noble' with the
appropriate codename for 26.04).
In `@jobs/pending_payments.ts`:
- Line 16: The query condition using attempts: { $lt:
Number(process.env.PAYMENT_ATTEMPTS) } can produce NaN when
process.env.PAYMENT_ATTEMPTS is undefined; change it to guard and use parseInt
like other places: check process.env.PAYMENT_ATTEMPTS !== undefined and
parseInt(process.env.PAYMENT_ATTEMPTS, 10) before building the query (update the
query that references attempts and any surrounding logic in the pending_payments
job to only include the $lt clause when the parsed integer is valid).
- Line 165: The query uses attempts: { $lt: Number(process.env.PAYMENT_ATTEMPTS)
} which becomes NaN if PAYMENT_ATTEMPTS is undefined; update the condition to
first guard against undefined and then use parseInt for parsing (e.g., check
process.env.PAYMENT_ATTEMPTS !== undefined then const maxAttempts =
parseInt(process.env.PAYMENT_ATTEMPTS, 10) and use attempts: { $lt: maxAttempts
}); apply this change where the query is built (the attempts field in
jobs/pending_payments.ts) so the query skips if the env var is missing and
parses consistently.
---
Nitpick comments:
In @.github/workflows/integrate.yaml:
- Line 16: The CI change that updates the workflow image from the previous
Ubuntu (the line "image: 'ubuntu:26.04'") and related label changes (jammy →
noble) are unrelated to the mongoose upgrade; revert the workflow modification
in the integrate.yaml (restore the prior image value e.g. the earlier ubuntu
version and original distro label) so this PR only contains the mongoose bump,
and open a separate PR that contains the infrastructure changes (the image:
'ubuntu:26.04' update and any distro name changes) with its own description and
testing notes.
🪄 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: c68ccd1f-c91e-4879-95b6-7bbddcef4ae2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.env-sample.github/workflows/integrate.yamlbot/commands.tsbot/messages.tsbot/modules/community/commands.tsbot/modules/community/messages.tsbot/modules/community/scenes.tsbot/modules/dispute/actions.tsbot/modules/dispute/commands.tsbot/modules/orders/commands.tsbot/modules/orders/scenes.tsbot/modules/orders/takeOrder.tsbot/ordersActions.tsbot/scenes.tsbot/start.tsbot/validations.tsjobs/pending_payments.tslocales/de.yamllocales/en.yamllocales/es.yamllocales/fa.yamllocales/fr.yamllocales/it.yamllocales/ko.yamllocales/pt.yamllocales/ru.yamllocales/uk.yamlmodels/community.tsmodels/order.tsmodels/user.tspackage.jsontests/bot/validation.spec.tstsconfig.jsonutil/communityHelper.tsutil/index.ts
✅ Files skipped from review due to trivial changes (9)
- tsconfig.json
- locales/it.yaml
- locales/ru.yaml
- locales/fa.yaml
- locales/de.yaml
- locales/pt.yaml
- locales/ko.yaml
- .env-sample
- locales/es.yaml
🚧 Files skipped from review as they are similar to previous changes (18)
- util/communityHelper.ts
- locales/uk.yaml
- models/user.ts
- locales/fr.yaml
- bot/modules/orders/commands.ts
- bot/scenes.ts
- bot/modules/community/messages.ts
- locales/en.yaml
- models/order.ts
- bot/modules/community/scenes.ts
- util/index.ts
- bot/modules/dispute/commands.ts
- bot/modules/orders/scenes.ts
- bot/start.ts
- bot/messages.ts
- bot/commands.ts
- bot/validations.ts
- tests/bot/validation.spec.ts
Collaborator
|
@ToRyVand beware, you have pushed unrelated commits in this PR (from PR778), remove them please |
Staged upgrade through 7.x, 8.x, and 9.x verifying 147/147 tests pass at each major version. Breaking changes resolved: - Remove incorrect `_id: string` redeclarations from Document interfaces (UserDocument, IOrder, ICommunity); mongoose 8+ enforces ObjectId type - Add explicit `id: string` virtual to document interfaces (mongoose 9) - Replace `FilterQuery` with `QueryFilter` (renamed in mongoose 9) - Add `.toString()` on `_id` assignments and query filters across 11 files where ObjectId was used in String schema fields - Fix `getOrderCountByCommunity` return type: `number[]` → `Record<string, number>` - Convert `process.env.PAYMENT_ATTEMPTS` to Number in `$lt` query operators - Convert `tgUser.id` (number) to String in User.findOne query filter - Add `skipLibCheck: true` to tsconfig to bypass mongoose 9 internal type recursion bug in populate.d.ts Closes lnp2pBot#655
…ent queries Use parseInt with a fallback default instead of Number() to avoid NaN being passed to MongoDB $lt operators when the env var is not set.
ToRyVand
force-pushed
the
feat/issue-655-mongoose-upgrade
branch
from
ea19828 to
6c886b6
Compare
Author
|
Done! Rebased the branch onto |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bot/validations.ts (1)
47-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent type conversion for
tg_idfield.Line 47 queries with
String(tgUser.id), but line 51 saves withtg_id: tgUser.id(without string conversion). Iftg_idis stored as a string field, both operations should convert consistently.🔧 Proposed fix
if (!user && start) { user = new User({ - tg_id: tgUser.id, + tg_id: String(tgUser.id), username: tgUser.username, lang: ctx.i18n.locale(), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bot/validations.ts` around lines 47 - 51, The code uses inconsistent conversion for tg_id: the query uses String(tgUser.id) while the new User creation sets tg_id: tgUser.id; update the User creation to set tg_id with the same string conversion (e.g., String(tgUser.id) or tgUser.id.toString()) so the User.findOne lookup and the User constructor use the same type; ensure this change is applied where User is created (the new User({...}) block) and keep the same conversion wherever tg_id is read or written.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bot/validations.ts`:
- Around line 740-742: Replace the loose equality in the banned-user check by
using strict equality: inside the community.banned_users.some callback (the
arrow function referencing buser: IUsernameId and user._id), compare buser.id
and user._id.toString() with ===; ensure both operands are strings (call
toString() on user._id is correct) and update the expression in the some(...)
callback accordingly.
- Line 8: The import of QueryFilter from mongoose should be a type-only import
to satisfy TypeScript/ESLint rules and avoid emitting runtime code; replace the
existing import statement with a type import (use "import type { QueryFilter }
from 'mongoose';") wherever QueryFilter is imported in this module (e.g., the
top-level import in validations.ts) and ensure any related references remain
unchanged since they are type usages only.
---
Outside diff comments:
In `@bot/validations.ts`:
- Around line 47-51: The code uses inconsistent conversion for tg_id: the query
uses String(tgUser.id) while the new User creation sets tg_id: tgUser.id; update
the User creation to set tg_id with the same string conversion (e.g.,
String(tgUser.id) or tgUser.id.toString()) so the User.findOne lookup and the
User constructor use the same type; ensure this change is applied where User is
created (the new User({...}) block) and keep the same conversion wherever tg_id
is read or written.
🪄 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: a11b6fce-ac15-420d-b24f-3441a2a45ad5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
.github/workflows/integrate.yamlbot/commands.tsbot/messages.tsbot/modules/community/commands.tsbot/modules/community/messages.tsbot/modules/community/scenes.tsbot/modules/dispute/actions.tsbot/modules/dispute/commands.tsbot/modules/orders/commands.tsbot/modules/orders/takeOrder.tsbot/ordersActions.tsbot/scenes.tsbot/start.tsbot/validations.tsjobs/pending_payments.tsmodels/community.tsmodels/order.tsmodels/user.tspackage.jsontsconfig.jsonutil/communityHelper.tsutil/index.ts
✅ Files skipped from review due to trivial changes (1)
- tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (18)
- bot/modules/community/messages.ts
- bot/modules/community/scenes.ts
- models/community.ts
- models/order.ts
- package.json
- bot/modules/orders/commands.ts
- bot/modules/orders/takeOrder.ts
- bot/ordersActions.ts
- util/index.ts
- jobs/pending_payments.ts
- bot/scenes.ts
- bot/modules/community/commands.ts
- models/user.ts
- bot/commands.ts
- .github/workflows/integrate.yaml
- bot/modules/dispute/commands.ts
- bot/modules/dispute/actions.ts
- bot/start.ts
| } from './start'; | ||
| import { IUsernameId } from '../models/community'; | ||
| import { FilterQuery } from 'mongoose'; | ||
| import { QueryFilter } from 'mongoose'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
mongoose 9.6.2 QueryFilter type import
💡 Result:
In Mongoose 9.x (including 9.6.2), the query filter type is exported as QueryFilter from the top-level mongoose package, so you import it like this: import type { QueryFilter } from 'mongoose'; You can then use it as QueryFilter. Mongoose’s type definitions show QueryFilter is the type of the first argument (filter) for Query#find and Query#getFilter/getQuery [1]. Example: import type { QueryFilter } from 'mongoose'; type MyDoc = { name: string }; const filter: QueryFilter = { name: { $regex: 'abc' } }; Notes: - In Mongoose 9, FilterQuery was renamed to QueryFilter [2]. - If you run into TypeScript assignability issues with QueryFilter generics (common around non-ObjectId _id and $match typing), Mongoose has ongoing fixes; e.g., PR #16020 adjusts QueryFilter typings so QueryFilter<...> is assignable where QueryFilter is expected (notably aggregation $match) [3].
Citations:
- 1: https://cdn.jsdelivr.net/npm/mongoose@9.1.2/types/query.d.ts
- 2: https://mongoosejs.com/docs/migrating_to_9.html
- 3: types(query): allow assigning QueryFilter<DocType> to QueryFilter<any> Automattic/mongoose#16020
🏁 Script executed:
# Check the content around line 8 in bot/validations.ts
head -20 bot/validations.tsRepository: lnp2pBot/bot
Length of output: 1903
Use import type for type-only imports per TypeScript best practices.
The QueryFilter name is correct for Mongoose 9.6.2 (it replaced FilterQuery), but the import should use import type { QueryFilter } from 'mongoose'; to follow ESLint TypeScript rules and avoid unnecessary runtime overhead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bot/validations.ts` at line 8, The import of QueryFilter from mongoose should
be a type-only import to satisfy TypeScript/ESLint rules and avoid emitting
runtime code; replace the existing import statement with a type import (use
"import type { QueryFilter } from 'mongoose';") wherever QueryFilter is imported
in this module (e.g., the top-level import in validations.ts) and ensure any
related references remain unchanged since they are type usages only.
Author
|
Hey @knocte, addressed the remaining CodeRabbit suggestions:
|
Collaborator
The only reason I intervened in this PR is because ubuntu24.04 has NodeJS18.x by default. If you raise the mongoose version to a version that requires NodeJS version higher than 18.x then you NEED to update CI to use ubuntu 26.04, not 24.04. Whatever MongoDB story you or the AI is telling, should be addressed around the MongoDB installation thingy. |
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.
Summary
Breaking changes resolved
mongoose 8.x
_id: stringredeclarations from UserDocument, IOrder, ICommunity — mongoose 8 enforces ObjectId.toString()in 11 files where ObjectId was used in String schema fields or query filtersmongoose 9.x
id: stringvirtual to document interfaces (no longer auto-inherited from Document)FilterQuerywithQueryFilter(renamed in mongoose 9)getOrderCountByCommunityreturn type:number[]toRecord<string, number>process.env.PAYMENT_ATTEMPTStoNumber()in$ltoperatorstgUser.id(number) toString()in User.findOne queryskipLibCheck: trueto tsconfig to bypass mongoose 9 internal type recursion bug in populate.d.tsTest plan
npm test— 147/147 passing with mongoose 9.6.2npx tsc --noEmit— zero errors (production build)npm run lint— zero errors in TypeScript source filesCloses #655
Summary by CodeRabbit
Chores
Refactor
Bug Fixes