Skip to content

Conversation

@RamzArzFarsi
Copy link

@RamzArzFarsi RamzArzFarsi commented Dec 12, 2025

Three small mistakes are fixed.

Summary by CodeRabbit

  • New Features

    • Buy flow now proceeds to a post-payment step, requires seller confirmation before sats are sent, and updates the follow-up command.
  • Bug Fixes

    • Adjusted image-processing message wording and normalized newline handling.
  • Documentation

    • Clarified sell guidance to show surplus in sats and made minor wording and label refinements.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 12, 2025

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

💤 Files selected but had no reviewable changes (1)
  • locales/fa.yaml

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Updated the English localization file (locales/en.yaml) to change buy-flow follow-up from /release to /fiatsent and add a seller-confirmation step; corrected sell guidance to reference sats for surplus; and made minor wording/newline/label adjustments in various messages.

Changes

Cohort / File(s) Summary
Localization string updates
locales/en.yaml
Replaced /release with /fiatsent in buy flow instructions and added a seller-confirmation step; updated sell guidance to indicate sats for the surplus parameter; adjusted wording and newline/formatting in image-processing and other messages.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify the /fiatsent command/handler exists and matches the updated instruction.
  • Confirm the surplus wording aligns with how amounts are presented/validated elsewhere.

Possibly related PRs

Poem

🐰 I hopped through lines and tweaked a phrase,
Swapped a route and smoothed the maze,
Sats now speak where numbers play,
A seller nod before the pay,
Tiny hops, a brighter day. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'en.yaml' is vague and does not convey meaningful information about the specific changes made to the file. Use a descriptive title that summarizes the main changes, such as 'Fix buy flow follow-up command and correct sell format descriptions' or 'Update en.yaml with buy flow and sell format corrections'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
locales/en.yaml (1)

428-435: Template variable rename creates breaking change: code still passes id but en.yaml expects orderId.

The code in bot/messages.ts (line 1854) passes id: order._id to render pending_payment_success, but the en.yaml template now expects ${orderId}. This mismatch will cause the order ID to render blank for English users. Synchronize the parameter name in the code to orderId to match the template, or revert the template variable back to ${id}.

Note: Other locale files (it, pt, ru, ko, uk, es, fa, fr, de) still use ${id} in their pending_payment_success templates, so only en.yaml was changed. This inconsistency across locales should also be addressed.

🧹 Nitpick comments (2)
locales/en.yaml (2)

25-25: Clarify /fiatsent usage (with/without order id) to match the bot’s actual command signature.
Right now the instructions say “use the /fiatsent command”, but /help documents /fiatsent <_order id_> while other copy shows /fiatsent without an id (e.g., fiatsent_order_cmd: /fiatsent). If the handler requires an id, this line should say /fiatsent ${orderId} (or explicitly mention selecting the order in-chat).


82-88: Wording is still ambiguous (“add the parameter 0 indicating sats amount”).
Consider tightening to “add 0 as the sats amount” and split the sentence for readability.

-  In order to create a sale with a surplus in the price of 3% \(premium\) you need to add the parameter 0 indicating sats amount\. The bot will calculate the selling price using the Bitcoin market price and applying the indicated surplus, you need to add 3 as the last parameter
+  To create a sale with a 3% premium, set the sats amount to 0\. The bot will calculate the sats amount from the market price and apply the premium\. Then add 3 as the last parameter\.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 875ede4 and a1fe355.

📒 Files selected for processing (1)
  • locales/en.yaml (4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: lnp2pBot/bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T14:03:55.114Z
Learning: Applies to {bot/**,locales/*.json} : Pair new bot flows with corresponding text updates under locales/
📚 Learning: 2025-08-14T18:53:14.640Z
Learnt from: CR
Repo: lnp2pBot/bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-14T18:53:14.640Z
Learning: Follow the Lightning Network hold-invoice escrow flow for buy and sell orders as documented

Applied to files:

  • locales/en.yaml

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
locales/en.yaml Outdated
Proof of payment: ${paymentSecret}
pending_payment_success: |
I have paid the lightning invoice for ${amount} satoshis, order ID: ${id}!
I have paid the lightning invoice for ${amount} satoshis, order ID: ${orderId}!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @RamzArzFarsi Thanks for your first contribution!
Was there a specific reason for changing it to ${orderId}?
${id} was used because this template serves dual purposes, it handles both order payments (order._id) and community payments (community.id).
Just want to make sure we maintain compatibility with both use cases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed the order ID placeholder was ${orderId} in the entire file except for this one particular place. I assumed it must be a typo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is not a typo

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it back.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old Persian file had weird phrasings and felt like a draft LLM translation.
This is a more refined and natural-sounding translation done manually (no LLM was used).

locales/fa.yaml Outdated

`/buy 1000 50000 IRT "پرداخت با عابربانک"`
`/sell 1000 300000 IRT "کارت به کارت"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be /buy, not sell

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed👍

Copy link
Member

@Catrya Catrya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @RamzArzFarsi , it won't compile; there are errors with the location keys. This is what appears when trying to compile:

[2025-12-15T16:52:25.134-06:00] error: Unhandled Rejection: YAMLException: can not read a block mapping entry; a multiline key may not be an implicit key (10:62)

  7 |  ... 
  8 |  ... ‌های پایاپای بیت‌کوین روی شبکه لایتنینگ کمک می‌کند.
  9 |  ... 
 10 |  ... د از دستورهای زیر استفاده کنید:
-----------------------------------------^
 11 |  ... 
 12 |  ... ستورهای /buy (خرید) یا /sell (فروش) منتشر کنید و از دستورالعم ... 

@RamzArzFarsi
Copy link
Author

Hi @RamzArzFarsi , it won't compile; there are errors with the location keys. This is what appears when trying to compile:

[2025-12-15T16:52:25.134-06:00] error: Unhandled Rejection: YAMLException: can not read a block mapping entry; a multiline key may not be an implicit key (10:62)

  7 |  ... 
  8 |  ... ‌های پایاپای بیت‌کوین روی شبکه لایتنینگ کمک می‌کند.
  9 |  ... 
 10 |  ... د از دستورهای زیر استفاده کنید:
-----------------------------------------^
 11 |  ... 
 12 |  ... ستورهای /buy (خرید) یا /sell (فروش) منتشر کنید و از دستورالعم ... 

The compiling error should be gone now. Please try again and let me know if there are any more errors.

@RamzArzFarsi RamzArzFarsi requested a review from Catrya December 16, 2025 10:13
Copy link
Contributor

@Luquitasjeffrey Luquitasjeffrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @RamzArzFarsi, I pulled your pr on my local and I'm still getting the same error:

[2025-12-17T18:40:51.873-03:00] error: Unhandled Rejection: YAMLException: can not read a block mapping entry; a multiline key may not be an implicit key (10:64)

7 | ...
8 | ... ‌های پایاپای بیت‌کوین روی شبکه لایتنینگ کمک می‌کند.
9 | ...
10 | ... د از دستورهای زیر استفاده کنید:
-----------------------------------------^
11 | ...
12 | ... ستورهای /buy (خرید) یا /sell (فروش) منتشر کنید و از دستورالعم ...

Please fix your syntax in the fa.yaml file so we can proceed with your pr.
Thanks

Copy link
Member

@Catrya Catrya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @Luquitasjeffrey said, it still doesn't work
The bot compiles but crashes on startup due to YAML syntax errors in fa.yaml. The translation file has invalid formatting that prevents the bot from starting. This needs to be fixed.

Updated Persian translations for disclaimer and instructions.
Corrected pluralization in Persian translations for various terms.
Copy link
Contributor

@Luquitasjeffrey Luquitasjeffrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now your pr is Ok. It can be merged to the main branch

@RamzArzFarsi
Copy link
Author

Please merge the PR if no further action is needed from my side.
And happy holidays to you. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants