fix: show sats amount on fixed orders - #852
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughOrder title generation now passes amount-related data into ChangesAmount-aware order title messages
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bot/ordersActions.ts (1)
286-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWhitespace baked into interpolated value is fragile.
amountTextembeds a trailing space (`${numberFormat(fiatCode, amount)} `) so that locale strings like'Selling ${amount}sats'render correctly. This couples the JS logic to an undocumented assumption about every locale template's spacing (no space beforesats/equivalent). A future locale edit that adds a space before the placeholder will silently produce a double space.Consider keeping
amountTextas just the formatted number (or empty) and letting each locale template own the spacing, e.g.${amount} satswith a conditional/trim, or exposing a separatehasAmountflag to the templates.♻️ Possible approach
- const amountText = priceFromAPI ? '' : `${numberFormat(fiatCode, amount)} `; + const amountText = priceFromAPI ? '' : `${numberFormat(fiatCode, amount)}`;Then update locale strings to
'Selling ${amount} sats'and have market/range variants use a dedicated string without the amount placeholder, instead of relying on an empty interpolated value.🤖 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 286 - 288, The spacing for order amount rendering is currently baked into `amountText` in `ordersActions.ts`, which makes locale templates fragile. Update the `amountText` handling in the order formatting logic so it only returns the formatted number or an empty string, and move spacing decisions into the locale strings used by the sell/order templates. Adjust the relevant template entries and any callers in the order action flow so fixed orders and market/range orders render correctly without relying on a trailing space inside `amountText`.
🤖 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.
Nitpick comments:
In `@bot/ordersActions.ts`:
- Around line 286-288: The spacing for order amount rendering is currently baked
into `amountText` in `ordersActions.ts`, which makes locale templates fragile.
Update the `amountText` handling in the order formatting logic so it only
returns the formatted number or an empty string, and move spacing decisions into
the locale strings used by the sell/order templates. Adjust the relevant
template entries and any callers in the order action flow so fixed orders and
market/range orders render correctly without relying on a trailing space inside
`amountText`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 672a802a-ca1c-40ff-83de-daf963947d86
📒 Files selected for processing (11)
bot/ordersActions.tslocales/de.yamllocales/en.yamllocales/es.yamllocales/fa.yamllocales/fr.yamllocales/it.yamllocales/ko.yamllocales/pt.yamllocales/ru.yamllocales/uk.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/bot/ordersActions.spec.ts (2)
17-97: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCoverage gaps: missing buy+username (fixed) and sell+username (market) cases.
The fixed-orders block covers sell/no-username, buy/no-username, sell/username, but not buy/username. The market/range block covers sell/no-username, buy/no-username, buy/username, but not sell/username. Since this suite is meant to guard against the amount-omission regression, filling in the missing permutations would give full confidence across both
isSellbranches and bothshow_usernamebranches.🤖 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/ordersActions.spec.ts` around lines 17 - 97, Add the missing permutation tests in ordersActions.spec for getOrderTitleMessage so both branches are covered: in the fixed orders describe block, add a publicUser buy case expecting the username plus “100 sats”, and in the market/range describe block, add a publicUser sell case expecting the username plus “sats” without an amount. Use the existing anonymousUser, publicUser, i18nCtx, and getOrderTitleMessage setup to mirror the surrounding assertions and cover both isSell and show_username combinations.
14-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
anyfor test fixtures; type againstUserDocument.Using a typed partial (e.g.,
Partial<UserDocument>or a minimal interface) instead ofanywould catch fixture drift ifUserDocument's shape changes.🤖 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/ordersActions.spec.ts` around lines 14 - 15, Replace the `any`-typed test fixtures in `ordersActions.spec.ts` with a typed `Partial<UserDocument>` (or a minimal fixture interface) for `anonymousUser` and `publicUser`. Update the fixture declarations near the `UserDocument`-related test setup so the objects are checked against the expected shape, keeping only the fields needed by the tests. This will make the `UserDocument` fixtures in the spec resilient to drift without relying on `any`.
🤖 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 `@tests/bot/ordersActions.spec.ts`:
- Around line 1-3: The spec file is using CommonJS require() imports, which
violates the TypeScript/ESLint import style. Update the top-level imports in
ordersActions.spec.ts to use ES import syntax for expect, I18n, and
getOrderTitleMessage so the file follows Standard + TypeScript rules and keeps
type-checking intact.
---
Nitpick comments:
In `@tests/bot/ordersActions.spec.ts`:
- Around line 17-97: Add the missing permutation tests in ordersActions.spec for
getOrderTitleMessage so both branches are covered: in the fixed orders describe
block, add a publicUser buy case expecting the username plus “100 sats”, and in
the market/range describe block, add a publicUser sell case expecting the
username plus “sats” without an amount. Use the existing anonymousUser,
publicUser, i18nCtx, and getOrderTitleMessage setup to mirror the surrounding
assertions and cover both isSell and show_username combinations.
- Around line 14-15: Replace the `any`-typed test fixtures in
`ordersActions.spec.ts` with a typed `Partial<UserDocument>` (or a minimal
fixture interface) for `anonymousUser` and `publicUser`. Update the fixture
declarations near the `UserDocument`-related test setup so the objects are
checked against the expected shape, keeping only the fields needed by the tests.
This will make the `UserDocument` fixtures in the spec resilient to drift
without relying on `any`.
🪄 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: 6914f968-0d86-4aaa-a38c-d5a8ecbd102a
📒 Files selected for processing (2)
bot/ordersActions.tstests/bot/ordersActions.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- bot/ordersActions.ts
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review done. LGTM.
Verified
- Faithful restoration of the pre-#770 behavior: compared against
66e0004's parent — same guard (priceFromAPI), same formatting (numberFormat(fiatCode, amount)), same output for every case. - Market/range orders:
priceFromAPI = !amountincreateOrder, so orders withamount === 0(market and range, including range child orders) correctly omit the amount. numberFormatcan't returnfalsein this path:createOrdervalidatesgetCurrency(fiatCode)beforebuildDescription, andbuildDescriptionhas a single caller.- No orphaned call sites: nothing else consumes
selling_sats/buying_sats/showusername_*, so no literal${amount}can leak into output. All 10 locales updated consistently. - No retroactive impact: descriptions are persisted at order creation.
Added on this branch
tests/bot/ordersActions.spec.ts(c48da5a, f45572f): regression test forgetOrderTitleMessageusing a real i18n context againstlocales/en.yaml— the #770 regression lived exactly in the code↔YAML${amount}interpolation contract, which a stubbedi18n.twouldn't catch. Covers fixed vs market/range orders, with and withoutshow_username. Full suite: 150 passing, lint clean.
Follow-ups filed separately (pre-existing, not blockers)
- #854 — fragile
${amount}-with-embedded-space i18n pattern (+ two minor code nits ingetOrderTitleMessage) - #855 —
numberFormat()returnsfalse | number | string - #856 —
ru.yaml"сат"/"саты" wording inconsistency
Thanks @Catrya!
2 participants
PR #770 moved the order title into a single i18n string and dropped the sats amount, so fixed orders showed "Selling sats" instead of "Selling 100 sats".
Restores the amount via a ${amount} placeholder in the locale keys (all languages) and computes it in getOrderTitleMessage: shown for fixed orders, omitted for market/range orders.
Summary by CodeRabbit
${amount}consistently (including username variants).