Update Lightning to 11.0.0 and TypeScript to ^5.9.3 Issue726#727
Open
Matobi98 wants to merge 3 commits intolnp2pBot:mainfrom
Open
Update Lightning to 11.0.0 and TypeScript to ^5.9.3 Issue726#727Matobi98 wants to merge 3 commits intolnp2pBot:mainfrom
Matobi98 wants to merge 3 commits intolnp2pBot:mainfrom
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 ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis pull request updates three dependencies in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 67: Update the `@typescript-eslint` packages to a newer 8.x release that
supports TypeScript 5.9.3: change the versions for `@typescript-eslint/parser` and
`@typescript-eslint/eslint-plugin` in package.json devDependencies to a current
8.x version that explicitly supports TS >=4.8.4 <6.0.0, then reinstall
(npm/yarn) to update lockfile; verify eslint still runs and lints files
referencing TypeScript by running your lint script or eslint directly.
Collaborator
Can you specify which fixes and improvements are the ones you're after? |
Contributor
Author
|
It was a mistake; I've updated the description to clarify the reason:
|
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review Summary
✅ Checks Passed
- CI Status: All workflows pass (Lint, NodeJS CI, CodeQL)
- Breaking Change Compatibility:
lightningv11.0.0 only breaks Node.js 18 support (requires ≥20). The project already uses Node.js 20 in Dockerfile and hasengines: {"node": ">=18.0.0"}, so this is compatible. - TypeScript Upgrade: 5.1.6 → 5.9.3 is a safe major bump with no known breaking changes for this codebase.
- ESLint Plugins:
@typescript-eslint/*8.11.0 → 8.21.0 is a minor version bump. - Lockfile:
package-lock.jsoncorrectly regenerated.
⚠️ Issues Found
-
Missing newline at EOF in
package.json- The diff shows
}without trailing newline:
-} +} \ No newline at end of file
- This violates POSIX standards and can cause issues with some tools.
- Fix required: Add a newline at the end of
package.json.
- The diff shows
-
Version specifier inconsistency
lightningchanged from exact (10.25.0) to caret (^11.0.0)- Other deps use exact versions (e.g.,
mongoose: 6.13.6,invoices: 2.0.6) - Recommendation: Use exact version
11.0.0for consistency with existing patterns, OR document the reason for allowing minor updates.
-
engines.node should be updated
- Current:
>=18.0.0 - With
lightningv11.0.0 requiring Node.js ≥20, theenginesfield should be updated to>=20.0.0to prevent installation on unsupported Node versions. - Fix recommended: Update
engines.nodeto>=20.0.0.
- Current:
📋 Checklist
- PR linked to issue #726
- CI passes
- No breaking runtime changes (Node 20 already in use)
- EOF newline in package.json
- Consider exact version for lightning
- Update engines.node to >=20.0.0
Verdict
Request Changes — The missing EOF newline is a clear issue. The engines.node update is strongly recommended to prevent future issues when someone tries to run on Node 18.
Collaborator
But Ubuntu LTS (24.04) still has 18.x. I recommend waiting until 26.04 is out the door to make this change (in April this year). |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
jobs/node_info.ts (1)
16-26:⚠️ Potential issue | 🟡 MinorRisk of runtime error if
getInfo()returnsundefined.Per
ln/info.ts,getInfo()returnsundefinedwhen an error occurs. Line 16 (info.is_synced_to_chain) will throw aTypeErrorifinfois undefined, preventing the config from being updated. The new lines 20-26 would also fail for the same reason.Consider adding a guard:
🛡️ Suggested fix
const info = await getInfo(); + if (!info) { + logger.warning('getInfo() returned undefined, skipping config update'); + return; + } if (info.is_synced_to_chain) { config.node_status = 'up'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jobs/node_info.ts` around lines 16 - 26, The code accesses properties on the result of getInfo() (info) without guarding for undefined, so add a null check before using info (e.g., in the job that calls getInfo()); if info is falsy, log the error and return/skip updating config, otherwise proceed to set config.node_status, config.node_uri, config.node_alias, config.node_version, config.node_block_height, config.node_channels_count, config.node_peers_count, config.node_synced_to_chain and config.node_synced_to_graph; alternatively use optional chaining and sensible defaults (info?.uris?.[0] ?? '', info?.current_block_height ?? 0, etc.) when assigning those fields to avoid TypeError from info being undefined.bot/modules/dispute/commands.ts (2)
23-29:⚠️ Potential issue | 🟠 MajorReject non-numeric
DISPUTE_START_WINDOWvalues.
parseIntletsNaNthrough here. Oncetimebecomes invalid, Line 29's comparison falls through and users can start disputes immediately instead of waiting for the configured window.Suggested fix
- const disputStartWindow = process.env.DISPUTE_START_WINDOW; - if (disputStartWindow === undefined) - throw new Error('DISPUTE_START_WINDOW environment variable not defined'); - const secsUntilDispute = parseInt(disputStartWindow); + const disputeStartWindow = process.env.DISPUTE_START_WINDOW; + if (disputeStartWindow === undefined) { + throw new Error('DISPUTE_START_WINDOW environment variable not defined'); + } + const secsUntilDispute = Number(disputeStartWindow); + if (!Number.isInteger(secsUntilDispute) || secsUntilDispute < 0) { + throw new Error('DISPUTE_START_WINDOW must be a non-negative integer'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/modules/dispute/commands.ts` around lines 23 - 29, The parsing of DISPUTE_START_WINDOW using parseInt can produce NaN, making the subsequent date calculation invalid; update the code that reads disputStartWindow and computes secsUntilDispute to validate the parse result (e.g., const secsUntilDispute = parseInt(disputStartWindow, 10); if (Number.isNaN(secsUntilDispute) || secsUntilDispute < 0) throw new Error('DISPUTE_START_WINDOW must be a non-negative integer'); ) so that secsUntilDispute is guaranteed numeric before using it to adjust time and comparing order.taken_at (refer to variables disputStartWindow and secsUntilDispute and the date logic that sets time and compares order.taken_at).
40-48:⚠️ Potential issue | 🟠 MajorMake the
DISPUTEtransition atomic.Line 48 saves the order before the later
countDocuments()calls anddispute.save(). If any of those awaits rejects, the catch only logs, so you can end up with an order stuck inDISPUTEwithout a matching dispute record, plus partially applied ban side effects.Also applies to: 54-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/modules/dispute/commands.ts` around lines 40 - 48, The DISPUTE transition (setting order.previous_dispute_status, seller_dispute/buyer_dispute flags, status='DISPUTE', generating buyer_dispute_token/seller_dispute_token and calling order.save()) must be made atomic with the creation of the dispute record and any ban side-effects: wrap the sequence that includes order updates (where you set previous_dispute_status, order.status, tokens), the subsequent countDocuments() checks, dispute.save(), and ban logic into a single DB transaction/session (or use a two-phase commit equivalent) so that either all writes succeed or none are persisted; if your DB driver supports sessions start a session, pass it into order.save() and dispute.save(), run the countDocuments queries in the same transaction context, and abort/rollback the transaction on any error to avoid leaving the order stuck in DISPUTE without a dispute record or partial bans.
🧹 Nitpick comments (8)
bot/middleware/commands.ts (1)
8-19: Use camelCase variable names in this parser block.
re_next_arg,next_arg,quoted_arg,unquoted_arg, andquoted_partshould be renamed to camelCase to match the TypeScript naming rule.♻️ Suggested refactor
- const re_next_arg = + const reNextArg = /^\s*((?:(?:"(?:\\.|[^"])*")|(?:'[^']*')|\\.|\S)+)\s*(.*)$/; let args = []; let command; if (match !== null) { if (match[1]) { command = match[1]; } - let next_arg = ['', '', match[2] || '']; - while ((next_arg = re_next_arg.exec(next_arg[2])!)) { - let quoted_arg = next_arg[1]; - let unquoted_arg = ''; - while (quoted_arg.length > 0) { - if (/^"/.test(quoted_arg)) { - const quoted_part = /^"((?:\\.|[^"])*)"(.*)$/.exec(quoted_arg); + let nextArg = ['', '', match[2] || '']; + while ((nextArg = reNextArg.exec(nextArg[2])!)) { + let quotedArg = nextArg[1]; + let unquotedArg = ''; + while (quotedArg.length > 0) { + if (/^"/.test(quotedArg)) { + const quotedPart = /^"((?:\\.|[^"])*)"(.*)$/.exec(quotedArg); - if (!quoted_part) break; - unquoted_arg += quoted_part[1].replace(/\\(.)/g, '$1'); - quoted_arg = quoted_part[2]; - } else if (/^'/.test(quoted_arg)) { - const quoted_part = /^'([^']*)'(.*)$/.exec(quoted_arg); + if (!quotedPart) break; + unquotedArg += quotedPart[1].replace(/\\(.)/g, '$1'); + quotedArg = quotedPart[2]; + } else if (/^'/.test(quotedArg)) { + const quotedPart = /^'([^']*)'(.*)$/.exec(quotedArg); - if (!quoted_part) break; - unquoted_arg += quoted_part[1]; - quoted_arg = quoted_part[2]; - } else if (/^\\/.test(quoted_arg)) { - unquoted_arg += quoted_arg[1]; - quoted_arg = quoted_arg.substring(2); + if (!quotedPart) break; + unquotedArg += quotedPart[1]; + quotedArg = quotedPart[2]; + } else if (/^\\/.test(quotedArg)) { + unquotedArg += quotedArg[1]; + quotedArg = quotedArg.substring(2); } else { - unquoted_arg += quoted_arg[0]; - quoted_arg = quoted_arg.substring(1); + unquotedArg += quotedArg[0]; + quotedArg = quotedArg.substring(1); } } - args[args.length] = unquoted_arg; + args[args.length] = unquotedArg; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bot/middleware/commands.ts` around lines 8 - 19, The parser block uses snake_case identifiers (re_next_arg, next_arg, quoted_arg, unquoted_arg, quoted_part); rename them to camelCase (reNextArg, nextArg, quotedArg, unquotedArg, quotedPart) and update every reference in the block (the regex declaration, the while loop assignment/exec, the variable initializations and any later uses) so types, non-null assertions (e.g., exec(...)!) and string indexing (match[1], match[2]) remain unchanged; ensure you only rename identifiers and not change logic or types in the surrounding code.ln/subscribe_invoice.ts (3)
80-91: Code duplication in setTimeout blocks.The two
setTimeoutblocks (lines 80-91 and 116-125) are nearly identical. Consider extracting to a helper to reduce duplication and simplify maintenance.♻️ Suggested refactor: extract helper function
+ const doResubscribe = () => { + pendingReconnects.add(id); + setTimeout(() => { + logger.info(`Attempting to resubscribe invoice with hash ${id}`); + subscribeInvoice(bot, id, true) + .catch(resubErr => { + logger.error(`Failed to resubscribe invoice ${id}: ${resubErr}`); + }) + .finally(() => { + pendingReconnects.delete(id); + }); + }, 5000); + }; + // ... then replace both setTimeout blocks with: - pendingReconnects.add(id); - setTimeout(() => { - logger.info(`Attempting to resubscribe invoice with hash ${id}`); - subscribeInvoice(bot, id, true) - .catch(resubErr => { - logger.error( - `Failed to resubscribe invoice ${id}: ${resubErr}`, - ); - }) - .finally(() => { - pendingReconnects.delete(id); - }); - }, 5000); + doResubscribe();Also applies to: 116-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ln/subscribe_invoice.ts` around lines 80 - 91, Extract the duplicated setTimeout retry logic into a small helper (e.g., scheduleResubscribe) that accepts the invoice id and a retry flag; replace both anonymous setTimeout blocks with calls to scheduleResubscribe(id, true). The helper should log via logger.info, call subscribeInvoice(bot, id, true).catch(...) to log errors with logger.error, and always remove the id from pendingReconnects in .finally(); reference the existing subscribeInvoice, logger, and pendingReconnects symbols when implementing the helper.
244-246: Consider usinglockedOrderdirectly instead of mutating the stale input parameter.Line 246 patches only
statusonto the originalorderobject, butlockedOrderis the fresh copy from the database. Other fields onorder(e.g.,buyer_id,seller_id,type) could theoretically have changed between when the caller fetchedorderand when the lock acquiredlockedOrder.Using
lockedOrderdirectly for the rest of the flow would be safer and clearer.♻️ Suggested fix: use lockedOrder throughout
if (lockedOrder === null) return; - // Use the locked order for the rest of the flow - Object.assign(order, { status: lockedOrder.status }); - const buyerUser = await User.findOne({ _id: order.buyer_id }); + // Use the locked order (fresh from DB) for the rest of the flow + const buyerUser = await User.findOne({ _id: lockedOrder.buyer_id }); if (buyerUser === null) throw new Error('buyerUser was not found'); - const sellerUser = await User.findOne({ _id: order.seller_id }); + const sellerUser = await User.findOne({ _id: lockedOrder.seller_id }); if (sellerUser === null) throw new Error('sellerUser was not found');Then replace all subsequent references to
orderwithlockedOrderin the rest of the function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ln/subscribe_invoice.ts` around lines 244 - 246, The code mutates the stale input parameter `order` by only patching `status` from the freshly-locked `lockedOrder`; instead use the authoritative `lockedOrder` for the rest of the flow. Replace subsequent uses of `order` in this function with `lockedOrder` (or assign `const currentOrder = lockedOrder` right after the null check) so all fields (e.g., buyer_id, seller_id, type, status) reflect the locked DB state; update any helper calls or checks that currently reference `order` to use that `currentOrder`/`lockedOrder` variable.
20-22: Deduplication gap:pendingReconnectscheck not applied atsubscribeInvoiceentry point.The
pendingReconnectsguard at line 60 only prevents duplicate scheduling withinscheduleResubscribe. However,resubscribe_invoices.ts(line 29) callssubscribeInvoice(bot, invoice.id, true)directly, bypassing this check. IfresubscribeInvoicesruns while a reconnect is pending for the same hash, duplicate subscription streams will be created.While the mutex in
payHoldInvoiceprevents double payment, duplicate streams waste resources and produce confusing logs.♻️ Suggested fix: add early return at subscribeInvoice entry
const subscribeInvoice = async ( bot: HasTelegram, id: string, resub: boolean = false, ) => { try { + // Skip if a reconnect is already pending for this invoice + if (pendingReconnects.has(id)) { + logger.info( + `subscribeInvoice: reconnect already pending for hash ${id}, skipping`, + ); + return; + } + const sub = subscribeToInvoice({ id, lnd });Also applies to: 59-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ln/subscribe_invoice.ts` around lines 20 - 22, Add an early dedupe check in subscribeInvoice so it returns immediately when a reconnect is already pending for that invoice hash: at the top of the subscribeInvoice function check pendingReconnects.has(invoiceHash) (the same Set used by scheduleResubscribe) and bail out (no new stream) if present; this ensures calls from resubscribe_invoices.ts (which calls subscribeInvoice(bot, invoice.id, true)) cannot create duplicate subscription streams and complements the existing scheduleResubscribe logic and the mutex in payHoldInvoice.monitoring.ts (2)
161-164: Prefer top-levelimportover inlinerequire().The
node-scheduleimport usesrequire()inline at runtime, which is inconsistent with the ES module imports at the top of the file and bypasses TypeScript's type checking.♻️ Suggested refactor
import axios from 'axios'; import mongoose from 'mongoose'; +import schedule from 'node-schedule'; import { logger } from './logger'; import { Config } from './models';Then update line 161-164:
- const schedule = require('node-schedule'); - schedule.scheduleJob('*/2 * * * *', async () => { + schedule.scheduleJob('*/2 * * * *', async () => { await sendHeartbeat(url, authToken, botName).catch(() => {}); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitoring.ts` around lines 161 - 164, Replace the inline require('node-schedule') with a top-level ES module import so TypeScript can type-check and keep imports consistent; update the file to import the schedule module (or scheduleJob) at the top and then call schedule.scheduleJob(...) exactly where sendHeartbeat(url, authToken, botName) is currently scheduled, keeping the same cron string and error handling. Ensure the imported symbol name matches the existing usage (e.g., schedule or scheduleJob) and that TypeScript types are satisfied.
158-158: Redundant.catch(() => {})on initial heartbeat.
sendHeartbeatalready catches and logs errors internally (lines 119-125), so the outer.catch(() => {})is redundant and silently swallows any unexpected errors that might escape the internal try-catch.♻️ Suggested simplification
// Send first heartbeat immediately - sendHeartbeat(url, authToken, botName).catch(() => {}); + sendHeartbeat(url, authToken, botName);The same applies to line 163 inside the scheduled job.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitoring.ts` at line 158, Remove the redundant .catch(() => {}) wrappers around sendHeartbeat calls so errors are not silently swallowed; sendHeartbeat already handles and logs its own errors, so locate the initial call sendHeartbeat(url, authToken, botName) and the one inside the scheduled job (the timer/cron callback) and delete the trailing .catch(() => {}) from each call, leaving the bare await or promise call so any unexpected exceptions can propagate or be handled by sendHeartbeat's internal try/catch and logging.tests/monitoring.spec.ts (1)
1-7: Consider consistent import style.The file mixes ES module imports (
import axios,import mongoose) with CommonJS requires (const { expect } = require('chai')). While functional, consistent usage of one style improves readability.♻️ Suggested consistency
import axios from 'axios'; import mongoose from 'mongoose'; +import { expect } from 'chai'; +import sinon from 'sinon'; +import proxyquire from 'proxyquire'; -const { expect } = require('chai'); -const sinon = require('sinon'); - -const proxyquire = require('proxyquire').noCallThru(); +const proxyquireNoCallThru = proxyquire.noCallThru();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/monitoring.spec.ts` around lines 1 - 7, The file mixes ES module imports (axios, mongoose) with CommonJS requires (expect, sinon, proxyquire); switch to consistent ES imports by replacing the require usages for { expect } from 'chai', sinon, and proxyquire with equivalent import statements, and if you need proxyquire.noCallThru() preserve that call by calling it on the imported proxyquire (e.g., import proxyquire and then invoke proxyquire.noCallThru()), updating any local variable names (expect, sinon, proxyquire) accordingly so all imports use the same ES module style.models/community.ts (1)
38-38: PreferTypes.ObjectIdforICommunity._idinstead ofstring.The interface extends Mongoose
Document, which usesTypes.ObjectIdfor_idby default. Typing it asstringmisrepresents the actual stored type and reduces type safety when working with Mongoose operations.♻️ Proposed typing adjustment
export interface ICommunity extends Document { - _id: string; + _id: Types.ObjectId; name: string; ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/community.ts` at line 38, Update the ICommunity interface to use Mongoose's Types.ObjectId for the _id field instead of string: locate the ICommunity interface in models/community.ts and change the type of _id from string to Types.ObjectId (importing Types from mongoose if not already imported) so the interface aligns with Mongoose Document's actual _id type and preserves type-safety in functions that use ICommunity.
🤖 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/modules/dispute/commands.ts`:
- Around line 98-104: The new branch that treats any non-empty
ctx.state.command.args as an explicit order id misses the temporary parser
workaround that treats the literal string 'undefined' as not-an-id; update the
condition that decides to call validateParams/handleDispute to also check
ctx.state.command.args[0] !== 'undefined' (or perform the same check right
before extracting [orderId]) so that the logic in commands.ts continues to
ignore the literal 'undefined' and falls back to the order picker; reference
ctx.state.command.args, validateParams, and handleDispute when making this
change.
In `@ln/subscribe_invoice.ts`:
- Around line 128-135: The comment claims "'error' is logged only to avoid
duplicate scheduling" but there is no sub.on('error', ...) handler, so add an
error handler on the same subscription object (sub.on('error', (err) => {...}))
that logs the error with context (use logger.error and include id and err) and
invokes scheduleResubscribe('error')—or, if you intentionally removed error
handling, update the comment to reflect that only 'end' triggers resubscribe;
reference the existing sub.on('end', ...) callback, the scheduleResubscribe
function, and the logger.warning usage so the change is colocated and
consistent.
In `@models/order.ts`:
- Around line 3-4: The IOrder interface currently redeclares "_id: string" which
overrides Mongoose's Document and strips off Document methods and the id getter;
remove the explicit "_id: string" declaration from the IOrder interface (or
alternatively extend Document with the correct types) so that IOrder properly
inherits Mongoose's Document methods and accessors (e.g., id, save(),
toObject()); update the IOrder declaration to simply extend Document without
redeclaring _id to restore compilation and existing usages of order.id and
order.save().
---
Outside diff comments:
In `@bot/modules/dispute/commands.ts`:
- Around line 23-29: The parsing of DISPUTE_START_WINDOW using parseInt can
produce NaN, making the subsequent date calculation invalid; update the code
that reads disputStartWindow and computes secsUntilDispute to validate the parse
result (e.g., const secsUntilDispute = parseInt(disputStartWindow, 10); if
(Number.isNaN(secsUntilDispute) || secsUntilDispute < 0) throw new
Error('DISPUTE_START_WINDOW must be a non-negative integer'); ) so that
secsUntilDispute is guaranteed numeric before using it to adjust time and
comparing order.taken_at (refer to variables disputStartWindow and
secsUntilDispute and the date logic that sets time and compares order.taken_at).
- Around line 40-48: The DISPUTE transition (setting
order.previous_dispute_status, seller_dispute/buyer_dispute flags,
status='DISPUTE', generating buyer_dispute_token/seller_dispute_token and
calling order.save()) must be made atomic with the creation of the dispute
record and any ban side-effects: wrap the sequence that includes order updates
(where you set previous_dispute_status, order.status, tokens), the subsequent
countDocuments() checks, dispute.save(), and ban logic into a single DB
transaction/session (or use a two-phase commit equivalent) so that either all
writes succeed or none are persisted; if your DB driver supports sessions start
a session, pass it into order.save() and dispute.save(), run the countDocuments
queries in the same transaction context, and abort/rollback the transaction on
any error to avoid leaving the order stuck in DISPUTE without a dispute record
or partial bans.
In `@jobs/node_info.ts`:
- Around line 16-26: The code accesses properties on the result of getInfo()
(info) without guarding for undefined, so add a null check before using info
(e.g., in the job that calls getInfo()); if info is falsy, log the error and
return/skip updating config, otherwise proceed to set config.node_status,
config.node_uri, config.node_alias, config.node_version,
config.node_block_height, config.node_channels_count, config.node_peers_count,
config.node_synced_to_chain and config.node_synced_to_graph; alternatively use
optional chaining and sensible defaults (info?.uris?.[0] ?? '',
info?.current_block_height ?? 0, etc.) when assigning those fields to avoid
TypeError from info being undefined.
---
Nitpick comments:
In `@bot/middleware/commands.ts`:
- Around line 8-19: The parser block uses snake_case identifiers (re_next_arg,
next_arg, quoted_arg, unquoted_arg, quoted_part); rename them to camelCase
(reNextArg, nextArg, quotedArg, unquotedArg, quotedPart) and update every
reference in the block (the regex declaration, the while loop assignment/exec,
the variable initializations and any later uses) so types, non-null assertions
(e.g., exec(...)!) and string indexing (match[1], match[2]) remain unchanged;
ensure you only rename identifiers and not change logic or types in the
surrounding code.
In `@ln/subscribe_invoice.ts`:
- Around line 80-91: Extract the duplicated setTimeout retry logic into a small
helper (e.g., scheduleResubscribe) that accepts the invoice id and a retry flag;
replace both anonymous setTimeout blocks with calls to scheduleResubscribe(id,
true). The helper should log via logger.info, call subscribeInvoice(bot, id,
true).catch(...) to log errors with logger.error, and always remove the id from
pendingReconnects in .finally(); reference the existing subscribeInvoice,
logger, and pendingReconnects symbols when implementing the helper.
- Around line 244-246: The code mutates the stale input parameter `order` by
only patching `status` from the freshly-locked `lockedOrder`; instead use the
authoritative `lockedOrder` for the rest of the flow. Replace subsequent uses of
`order` in this function with `lockedOrder` (or assign `const currentOrder =
lockedOrder` right after the null check) so all fields (e.g., buyer_id,
seller_id, type, status) reflect the locked DB state; update any helper calls or
checks that currently reference `order` to use that `currentOrder`/`lockedOrder`
variable.
- Around line 20-22: Add an early dedupe check in subscribeInvoice so it returns
immediately when a reconnect is already pending for that invoice hash: at the
top of the subscribeInvoice function check pendingReconnects.has(invoiceHash)
(the same Set used by scheduleResubscribe) and bail out (no new stream) if
present; this ensures calls from resubscribe_invoices.ts (which calls
subscribeInvoice(bot, invoice.id, true)) cannot create duplicate subscription
streams and complements the existing scheduleResubscribe logic and the mutex in
payHoldInvoice.
In `@models/community.ts`:
- Line 38: Update the ICommunity interface to use Mongoose's Types.ObjectId for
the _id field instead of string: locate the ICommunity interface in
models/community.ts and change the type of _id from string to Types.ObjectId
(importing Types from mongoose if not already imported) so the interface aligns
with Mongoose Document's actual _id type and preserves type-safety in functions
that use ICommunity.
In `@monitoring.ts`:
- Around line 161-164: Replace the inline require('node-schedule') with a
top-level ES module import so TypeScript can type-check and keep imports
consistent; update the file to import the schedule module (or scheduleJob) at
the top and then call schedule.scheduleJob(...) exactly where sendHeartbeat(url,
authToken, botName) is currently scheduled, keeping the same cron string and
error handling. Ensure the imported symbol name matches the existing usage
(e.g., schedule or scheduleJob) and that TypeScript types are satisfied.
- Line 158: Remove the redundant .catch(() => {}) wrappers around sendHeartbeat
calls so errors are not silently swallowed; sendHeartbeat already handles and
logs its own errors, so locate the initial call sendHeartbeat(url, authToken,
botName) and the one inside the scheduled job (the timer/cron callback) and
delete the trailing .catch(() => {}) from each call, leaving the bare await or
promise call so any unexpected exceptions can propagate or be handled by
sendHeartbeat's internal try/catch and logging.
In `@tests/monitoring.spec.ts`:
- Around line 1-7: The file mixes ES module imports (axios, mongoose) with
CommonJS requires (expect, sinon, proxyquire); switch to consistent ES imports
by replacing the require usages for { expect } from 'chai', sinon, and
proxyquire with equivalent import statements, and if you need
proxyquire.noCallThru() preserve that call by calling it on the imported
proxyquire (e.g., import proxyquire and then invoke proxyquire.noCallThru()),
updating any local variable names (expect, sinon, proxyquire) accordingly so all
imports use the same ES module style.
🪄 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: 56da29f2-0029-4fb9-9bdd-0d17448e790c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
.env-sample.github/workflows/release.yamlAGENTS.mdCHANGELOG.mdCONTRIBUTING.mdapp.tsbot/messages.tsbot/middleware/commands.tsbot/modules/community/actions.tsbot/modules/community/commands.tsbot/modules/dispute/actions.tsbot/modules/dispute/commands.tsbot/modules/dispute/index.tsbot/modules/dispute/messages.tsbot/ordersActions.tsdocs/monitoring.mdjobs/check_solvers.tsjobs/communities.tsjobs/delete_published_orders.tsjobs/node_info.tsln/subscribe_invoice.tslocales/de.yamllocales/en.yamllocales/es.yamllocales/fa.yamllocales/fr.yamllocales/it.yamllocales/ko.yamllocales/pt.yamllocales/ru.yamllocales/uk.yamlmodels/community.tsmodels/config.tsmodels/indexes.tsmodels/order.tsmodels/user.tsmonitoring.tspackage.jsontests/monitoring.spec.tsutil/index.ts
✅ Files skipped from review due to trivial changes (11)
- bot/ordersActions.ts
- .env-sample
- CHANGELOG.md
- docs/monitoring.md
- .github/workflows/release.yaml
- locales/es.yaml
- locales/pt.yaml
- AGENTS.md
- locales/fr.yaml
- models/config.ts
- CONTRIBUTING.md
Comment on lines
+98
to
+104
| if (ctx.state.command.args.length > 0) { | ||
| const params = await validateParams(ctx, 2, '\\<_order id_\\>'); | ||
|
|
||
| if (!params || params.length === 0) return; | ||
|
|
||
| const [orderId] = params; | ||
| return await handleDispute(ctx, orderId); |
Contributor
There was a problem hiding this comment.
Keep the literal 'undefined' workaround in the new no-arg branch.
This now treats 'undefined' as a real order id because it only checks args.length. When the known parser bug hits, /dispute will skip the order picker and go down the invalid-id path instead.
Suggested fix
- if (ctx.state.command.args.length > 0) {
+ const [firstArg] = ctx.state.command.args;
+ if (firstArg && firstArg !== 'undefined') {
const params = await validateParams(ctx, 2, '\\<_order id_\\>');
if (!params || params.length === 0) return;📝 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.
Suggested change
| if (ctx.state.command.args.length > 0) { | |
| const params = await validateParams(ctx, 2, '\\<_order id_\\>'); | |
| if (!params || params.length === 0) return; | |
| const [orderId] = params; | |
| return await handleDispute(ctx, orderId); | |
| const [firstArg] = ctx.state.command.args; | |
| if (firstArg && firstArg !== 'undefined') { | |
| const params = await validateParams(ctx, 2, '\\<_order id_\\>'); | |
| if (!params || params.length === 0) return; | |
| const [orderId] = params; | |
| return await handleDispute(ctx, orderId); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bot/modules/dispute/commands.ts` around lines 98 - 104, The new branch that
treats any non-empty ctx.state.command.args as an explicit order id misses the
temporary parser workaround that treats the literal string 'undefined' as
not-an-id; update the condition that decides to call
validateParams/handleDispute to also check ctx.state.command.args[0] !==
'undefined' (or perform the same check right before extracting [orderId]) so
that the logic in commands.ts continues to ignore the literal 'undefined' and
falls back to the order picker; reference ctx.state.command.args,
validateParams, and handleDispute when making this change.
Comment on lines
+128
to
+135
| // The stream's 'end' event is the single resubscribe trigger. | ||
| // 'error' is logged only to avoid duplicate scheduling from both events. | ||
| sub.on('end', () => { | ||
| logger.warning( | ||
| `subscribeInvoice stream ended for hash ${id}, attempting resubscription`, | ||
| ); | ||
| scheduleResubscribe('end'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's an 'error' event handler elsewhere in this file or if it was removed
rg -n "sub\.on\('error'" ln/subscribe_invoice.ts
# Check git diff for removed error handler
git log --oneline -n 5 --all -- ln/subscribe_invoice.ts | head -1 | xargs -I {} git show {} -- ln/subscribe_invoice.ts | grep -A5 "on('error'"Repository: lnp2pBot/bot
Length of output: 161
🏁 Script executed:
# First, check the file size
wc -l ln/subscribe_invoice.tsRepository: lnp2pBot/bot
Length of output: 83
🏁 Script executed:
# Read the entire file to understand the context
cat -n ln/subscribe_invoice.tsRepository: lnp2pBot/bot
Length of output: 13012
🏁 Script executed:
# Specifically search for any 'error' event handler on 'sub'
rg -n "error" ln/subscribe_invoice.ts -A2 -B2Repository: lnp2pBot/bot
Length of output: 1818
Add error event handler or clarify misleading comment.
The comment on lines 128–129 states "'error' is logged only to avoid duplicate scheduling," but no sub.on('error', ...) handler exists in this code. Without an error handler, unhandled stream errors will crash the application. Either add an error handler (similar to the 'end' handler) or update the comment if error handling was intentionally removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ln/subscribe_invoice.ts` around lines 128 - 135, The comment claims "'error'
is logged only to avoid duplicate scheduling" but there is no sub.on('error',
...) handler, so add an error handler on the same subscription object
(sub.on('error', (err) => {...})) that logs the error with context (use
logger.error and include id and err) and invokes
scheduleResubscribe('error')—or, if you intentionally removed error handling,
update the comment to reflect that only 'end' triggers resubscribe; reference
the existing sub.on('end', ...) callback, the scheduleResubscribe function, and
the logger.warning usage so the change is colocated and consistent.
Comment on lines
3
to
+4
| export interface IOrder extends Document { | ||
| _id: string; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for IOrder._id usage patterns to verify string vs ObjectId expectations
rg -n -C3 'order\._id' --type=ts | head -100
# Check if there are toString() conversions or string comparisons
rg -n 'order\._id\.toString\(\)' --type=tsRepository: lnp2pBot/bot
Length of output: 6734
🏁 Script executed:
cat -n models/order.ts | head -100Repository: lnp2pBot/bot
Length of output: 3500
🏁 Script executed:
git log --oneline -20 -- models/order.tsRepository: lnp2pBot/bot
Length of output: 139
🏁 Script executed:
git show --no-patch --format="" d2b6962:models/order.ts | head -60Repository: lnp2pBot/bot
Length of output: 1548
🏁 Script executed:
git log --oneline --all -- models/order.ts | head -20Repository: lnp2pBot/bot
Length of output: 774
🏁 Script executed:
git show f6d9b5b -- models/order.ts | head -100Repository: lnp2pBot/bot
Length of output: 1142
🏁 Script executed:
npx tsc --noEmit 2>&1 | grep -i "order\|_id" | head -30Repository: lnp2pBot/bot
Length of output: 3096
🏁 Script executed:
# Check if there are TypeScript errors specifically related to _id
rg -n "order\.id[^_]" --type=ts | head -20Repository: lnp2pBot/bot
Length of output: 698
🏁 Script executed:
# Check if there's a separate 'id' property or if it's a virtual
rg -n "\.id['\"]" models/order.tsRepository: lnp2pBot/bot
Length of output: 38
🏁 Script executed:
# Check the Mongoose Document type definition to see what properties it provides
node -e "const mongoose = require('mongoose'); const doc = new mongoose.Document(); console.log(Object.getOwnPropertyNames(Object.getPrototypeOf(doc)).filter(p => p.includes('id')).slice(0, 20));"Repository: lnp2pBot/bot
Length of output: 891
🏁 Script executed:
# Search for how Mongoose's Document defines id vs _id
npm list mongoose 2>/dev/null | head -5Repository: lnp2pBot/bot
Length of output: 103
🏁 Script executed:
# Check if there's documentation about Mongoose Document's id property
rg -n "Document.*id" --type=ts models/ | head -20Repository: lnp2pBot/bot
Length of output: 38
🏁 Script executed:
# Check if there are any custom definitions for 'id' property or if it comes from Document
rg -n "get id\(\)" --type=ts models/Repository: lnp2pBot/bot
Length of output: 38
🏁 Script executed:
# Look at the actual TypeScript compilation errors related to IOrder more carefully
npx tsc --noEmit 2>&1 | grep -A2 "IOrder"Repository: lnp2pBot/bot
Length of output: 5249
🏁 Script executed:
# Check what properties and methods the actual usage expects from IOrder
rg -n "order\.(id|save|toObject)" --type=ts | head -30Repository: lnp2pBot/bot
Length of output: 1779
🏁 Script executed:
# Check how Document type is imported and used elsewhere
rg -n "Document" models/ --type=tsRepository: lnp2pBot/bot
Length of output: 1564
🏁 Script executed:
# Check if Document interface is supposed to provide save() and id
rg -n "extends Document" models/ -A20 --type=ts | grep -E "(save|id)" | head -20Repository: lnp2pBot/bot
Length of output: 942
The _id: string declaration breaks TypeScript compilation and loses critical Document interface properties.
Adding _id: string to the IOrder interface causes compilation errors throughout the codebase. The explicit declaration overrides Mongoose's Document type, which provides:
- The
idgetter (used in ~10+ files asorder.id) - Document methods like
save()andtoObject()(used extensively across the codebase)
Mongoose's Document interface already declares _id with a proper getter and the id accessor. Either remove the explicit _id: string declaration to preserve Document inheritance, or extend Document with the missing methods and properties.
The current state fails TypeScript compilation with errors like "Property 'save' does not exist on type 'IOrder'" and "Property 'id' does not exist on type 'IOrder'".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models/order.ts` around lines 3 - 4, The IOrder interface currently
redeclares "_id: string" which overrides Mongoose's Document and strips off
Document methods and the id getter; remove the explicit "_id: string"
declaration from the IOrder interface (or alternatively extend Document with the
correct types) so that IOrder properly inherits Mongoose's Document methods and
accessors (e.g., id, save(), toObject()); update the IOrder declaration to
simply extend Document without redeclaring _id to restore compilation and
existing usages of order.id and order.save().
Lightint to 11.0.2 Typescript to 5.9.3 bn.js to 5.2.3
bn.js to 5.2.3 Typescript to 5.9.3
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
Updated the Lightning dependency to 11.0.0. This required upgrading TypeScript to ^5.9.3 (minimum required ≥ 5.4), so TypeScript was bumped to ^5.9.3.
Changes
lightningto 11.0.0 inpackage.json.typescriptto ^5.9.3.package-lock.json/yarn.lock) accordingly.Why
Summary by CodeRabbit