Repository navigation
feat(keyboard): fill amounts from the right on the on-screen keypad - #1074
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe keyboard now stores entered amounts as digits and formats them using the active mode’s decimal precision. It converts initial, maximum-balance, and toggled values into that representation, removes the decimal-point key, and updates component and end-to-end test coverage. ChangesKeypad Amount Entry
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 8 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: keypad amounts now fill from the right on the on-screen keyboard.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Deploying wallet-bitcoin with
|
| Latest commit: |
3741ebf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a436c9d2.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://split-p02-keypad-rtl.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
3741ebf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://290f75f2.arkade-wallet.pages.dev |
| Branch Preview URL: | https://split-p02-keypad-rtl.arkade-wallet.pages.dev |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — PR #1074 · keypad right-to-left entry
No protocol-critical paths touched (no VTXO/signing/forfeit/round/exit). Overall this is a clean, well-scoped change. The digit-model abstraction in keypadEntry.ts is solid and the 28-test unit suite gives good edge-case coverage. A few items worth addressing before the receive-redesign merge lands.
Issues
1. amountInSats effect: missing fiatDecimals-driven invalidation (src/components/Keyboard.tsx)
// new deps: [digits, inputMode]
useEffect(() => {
const strValue = textValue.replaceAll(',', '') // textValue derived from digits + fiatDecimals()
...
}, [digits, inputMode])textValue is digitsToValue(digits, getMaxDecimals()) and getMaxDecimals() calls fiatDecimals() when inputMode === 'fiat'. If the fiat context changes decimal precision (e.g., USD→JPY mid-session) while digits and inputMode are unchanged, textValue will update on the next render but the effect won't fire — leaving amountInSats stale until the user types another digit or toggles mode.
In practice the currencyConversionUseful effect clears digits on a currency context change, so the window is narrow. But the dependency is technically incomplete. The safest minimal fix is to include textValue in the deps array:
}, [digits, inputMode, textValue])textValue is a render-derived string, so adding it is safe and makes the deps honest.
2. valueToDigits silently truncates excess fractional digits (src/lib/keypadEntry.ts, line 24)
frac.padEnd(decimals, '0').slice(0, decimals)If frac is longer than decimals the extra digits are dropped without warning. The test at keypadEntry.test.ts line 57 ('1.234567891' → '123456789' at 8 decimals) confirms this is intentional. All current callers produce strings with at most decimals fractional digits (via toFixed, prettyNumber, or atomicToUnits), so there is no current caller-side bug. The behaviour should be documented in the JSDoc since a future caller passing a raw float string could lose precision silently.
3. Existing form.test.ts / bip21.test.ts callers — verified correct, no action needed
handleKeyboardInput(page, 2100) (default decimals = 0): (2100).toFixed(0) → '2100' → same digit sequence as the old sats.toString() path. All sats-denominated callers that omit decimals behave identically to before. ✓
Observations (no action required)
btcPrimary regex (Keyboard.tsx, line 167): /^[0.]*/ correctly grays out the insignificant leading zeros/dot in BTC display. digitsToValue always produces well-formed strings so no surprising match is reachable in practice.
pushDigit with maxUnits (keypadEntry.ts, line 11): BigInt(digits + key) is safe because the guard digits === '' && key === '0' → return '' fires before this path, so digits + key is always a non-empty decimal string when BigInt() is called.
Asset mode has no satoshi cap — only maxEntryDigits bounds it. That is correct; the over-balance guard handles the asset-specific limit at display time.
handleToggleCurrency sets digits and inputMode in the same synchronous event — React 18 batches these, so the single re-render sees both values together. No tearing.
Nits
Keyboard.tsxline 170:getMaxDecimalsis called on every render to derivetextValue; auseMemooruseCallbackwould make the dependency chain clearer, though not a correctness concern for a keyboard component.keypadEntry.tsline 3 (maxEntryDigits) JSDoc could note that the 12-digit whole-part floor is intentional for 0-decimal (sats) mode.
|
@arkana-ai-bot thanks.
On the nits, I left both as they are:
|
|
Thanks, @Kukks — both fixes confirmed in
The rationale on the two nits makes sense. Correction on my end: I wrote "12-digit floor" in the Nothing blocking from my side. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana follow-up review — PR #1074 f21862a
One commit since last pass; both prior issues are now closed. No new findings.
Prior issue status
1. amountInSats effect missing textValue dependency — Fixed in f21862a.
Keyboard.tsx line 118: dependency array is now [digits, inputMode, textValue]. A precision change with unchanged digits will correctly recompute the amount.
2. valueToDigits silent truncation undocumented — Fixed in f21862a.
keypadEntry.ts line 24 comment now states that excess fraction digits past decimals are dropped. Contract is now visible to future callers.
No new concerns in this diff. Change is ready from my side.
|
@coderabbitai review |
✅ Action performedReview finished.
|
The phone keypad now enters digits right to left in the mode's smallest step, like a cash register: typing 1 2 in BTC shows 0.00000012, and in USD $0.12. There is no decimal-point key any more. Over-long values clamp instead of being divided, a digit that would pass 21 M BTC is refused, and a past-cap balance stays exact instead of turning into nines. The digit logic lives in lib/keypadEntry.ts with its own tests. The e2e helpers type amounts in the mode's smallest step: pass the mode's decimals (2 for USD) so 2 becomes $2.00 rather than $0.02. Split out of #1068.
…changes The sats/asset amount effect read textValue but only listed digits and inputMode, so a precision change (fiat decimals, asset decimals) with the same digits left the amount stale until the next key. textValue is now a dependency. valueToDigits' comment also notes that fraction digits past the precision are dropped.
f21862a to
3741ebf
Compare
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana follow-up review — PR #1074 · 3741ebf
Status: Branch was force-pushed / rebased onto a newer master since last pass (commits ad6235e1 SDK 0.5 upgrade and da59fe99 sdk rc.24/swap rc.27 are now in the branch history). The keyboard feature code itself is byte-for-byte identical to the previous reviewed tree. Both prior issues remain Fixed; no new findings.
Prior issue status
1. amountInSats effect: missing textValue dependency — Fixed (confirmed in rebased 3741ebf1).
Keyboard.tsx hunk line }, [digits, inputMode, textValue] is present in the rebased commit, same as f21862ac.
2. valueToDigits silent truncation undocumented — Fixed (confirmed in rebased 3741ebf1).
keypadEntry.ts: comment reads "excess fraction digits drop" — contract is visible to future callers, same as f21862ac.
Scope note on the large compare
gh api …/compare/f21862ac…3741ebf1 shows 117 files because f21862ac is no longer an ancestor of 3741ebf1 after the rebase; the symmetric diff pulls in everything from the SDK upgrade commits. The PR-level diff is still +265 / −100, all keyboard-scoped. The non-keyboard files are from #936 and #1087, already merged to master and separately reviewed there; they are out of scope for this PR.
No new concerns. Change is ready from my side.
The phone keypad enters digits right to left in the mode's smallest step, like a cash register: 1 2
shows 0.00000012 BTC, or $0.12 in USD. There is no decimal-point key. Typing stops at the digit cap,
a digit that would pass 21 M BTC is refused, and a balance loaded past the cap stays exact. The digit
logic lives in
lib/keypadEntry.ts. A precision change with the same digits (a fiat or asset with otherdecimals) recomputes the amount at once, not on the next key.
The e2e helpers type amounts in the mode's smallest step:
handleKeyboardInput(page, value, decimals),and
prePay(..., decimals).title/subtitleprops, which Receive's "Set an amount first" uses; they come with thereceive redesign.
Test plan
pnpm test:unit: 102 files / 997 tests (CI at f21862a).Dependencies: none. The receive redesign merges this in.
Split out of #1068.
Split series — supersedes #1000 and #1068; each PR is one feature. Base
masterunless noted:Summary by CodeRabbit
199in a two-decimal currency displays1.99.