laantungir/minibits_wallet
1
0
Fork 0
mirror of https://github.com/minibits-cash/minibits_wallet.git synced 2026-07-22 07:48:28 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix truncated amount: derive the grouped display, never store it

Browse Source 
"1000" confirmed as "1,00". Two mechanisms, both from writing the grouped string
back into the controlled TextInput's value:

- Android echoes a programmatically-set `value` back out through onChangeText,
  where handleTopChange rewrites the first comma to a dot (a decimal-comma
  keyboard types "1,5" and means 1.5). So "1,000" came back as "1.000".

- maxLength was clipping. It caps typing, but Android's filter applies to
  programmatically-set text too, and the grouped form is longer than the number
  it shows.

Stripping on focus was not enough, because it only closed the window the USER
could reach — not the one the platform reaches on its own.

State now holds the plain number and the grouped text is derived at render time,
only while the field is unfocused. Losing focus is what confirms an amount, so
that alone produces the requested behaviour, and derived text cannot be read back
in as input. The typing cap moves from maxLength into the change handlers, where
it applies to typing and nothing else. Parents keep receiving the plain number, so
nothing downstream has to know about any of this.

87 tsc (unchanged baseline), 335/335.
This commit is contained in:
minibits-cash
2026-07-14 23:42:01 +02:00
parent c96c4ebfef
commit 5e8932d411
2 changed files with 129 additions and 118 deletions
Download Patch File Download Diff File Expand all files Collapse all files

137
__tests__/amountFormatting.test.ts
View File

@@ -1,99 +1,96 @@
/**
* Grouping-separator round trip for confirmed amounts.
* Grouping separators on confirmed amounts.
*
* AmountInput now GROUPS an amount once the user confirms it ("12,345") and STRIPS the
* grouping again while it is being edited. That only works if every consumer of the
* amount string can read a grouped number back — and every screen parses it with
* `toNumber`, so that is the contract worth pinning.
* AmountInput SHOWS a confirmed amount grouped ("12,345") but never STORES it that way:
* state holds the plain number and the grouped text is derived at render time, only while
* the field is unfocused.
*
* The reason the strip half exists is subtler and is pinned here too: AmountInput
* rewrites a comma to a dot on input, so a decimal-comma keyboard can type "1,5" and
* mean 1.5. If a grouped value were ever left sitting in a focused field, editing
* "12,345" would silently reinterpret it as 12.345 — a hundredfold error, in a field
* whose entire job is to say how much money to move.
* That split is the whole design, and this pins why it has to be. These are controlled
* TextInputs, and AmountInput rewrites the first comma to a dot on input so a
* decimal-comma keyboard can type "1,5" and mean 1.5. Keep a grouped string in state and
* that rewrite gets a chance at it: "1,000" becomes "1.000", which is 1 — a thousandfold
* error, silent, in the field that decides how much money leaves the wallet. Derived
* display cannot be read back in as input, so the two meanings of "," never meet.
*
* @jest-environment node
*/
import {formatNumber, toNumber} from '../src/utils/number'
/** What AmountInput does on focus. */
const stripGrouping = (v: string) => v.replace(/,/g, '')
/** What AmountInput renders when a field is unfocused. Never written to state. */
const formatForDisplay = (v: string, mantissa: number) => {
const n = toNumber(v.replace(/,/g, ''))
if (n === undefined || !Number.isFinite(n)) return v
return formatNumber(n, mantissa)
}
/** What AmountInput does on input, before the value reaches state. */
/** What AmountInput does to every keystroke, before it reaches state. */
const normalizeDecimalComma = (v: string) => v.replace(',', '.')
describe('toNumber reads grouped amounts', () => {
it('parses what formatNumber produces', () => {
expect(toNumber('12,345')).toBe(12345)
expect(toNumber('1,234,567')).toBe(1234567)
expect(toNumber('999,999,999')).toBe(999999999)
})
it('still parses ungrouped amounts, so nothing regresses mid-edit', () => {
expect(toNumber('12345')).toBe(12345)
expect(toNumber('0')).toBe(0)
})
it('parses a grouped fiat amount with a mantissa', () => {
expect(toNumber('1,234.56')).toBe(1234.56)
})
})
describe('formatNumber groups a confirmed amount to the unit precision', () => {
// sat: mantissa 0. Fractional sats do not exist, and the screens round to integers
// anyway before doing anything with the value.
describe('formatForDisplay groups a confirmed amount to the unit precision', () => {
// sat: mantissa 0. Fractional sats do not exist, and every screen rounds to an integer
// before doing anything with the value anyway.
it('groups sats without a mantissa', () => {
expect(formatNumber(12345, 0)).toBe('12,345')
expect(formatNumber(999999999, 0)).toBe('999,999,999')
expect(formatNumber(0, 0)).toBe('0')
expect(formatForDisplay('1000', 0)).toBe('1,000')
expect(formatForDisplay('12345', 0)).toBe('12,345')
expect(formatForDisplay('999999999', 0)).toBe('999,999,999')
expect(formatForDisplay('0', 0)).toBe('0')
})
// fiat: mantissa 2.
it('pads a fiat amount to two places', () => {
expect(formatNumber(1234.5, 2)).toBe('1,234.50')
expect(formatNumber(12, 2)).toBe('12.00')
expect(formatForDisplay('1234.5', 2)).toBe('1,234.50')
expect(formatForDisplay('12', 2)).toBe('12.00')
})
it('leaves unparseable text alone rather than blanking the field', () => {
expect(formatForDisplay('', 0)).toBe('')
})
// Screens pre-fill from BIP21 / melt quotes with numbro's thousandSeparated, so a
// grouped string can arrive from outside. Formatting it again must not compound.
it('is idempotent, so an already-grouped input survives', () => {
expect(formatForDisplay('1,000', 0)).toBe('1,000')
expect(formatForDisplay(formatForDisplay('12345', 0), 0)).toBe('12,345')
})
})
describe('round trip: confirm, then edit again', () => {
const confirm = (raw: string, mantissa: number) => formatNumber(toNumber(raw), mantissa)
it('survives confirm -> focus -> confirm without drifting', () => {
const confirmed = confirm('12345', 0)
expect(confirmed).toBe('12,345')
// Focus strips the grouping, so the field holds a plain number again...
const editing = stripGrouping(confirmed)
expect(editing).toBe('12345')
// ...and re-confirming lands on the same value rather than compounding.
expect(confirm(editing, 0)).toBe('12,345')
})
describe('the grouped form never reaches state', () => {
/**
* THE reason the value is stripped on focus.
* What storing the grouped value would have cost.
*
* If a grouped amount were left in a focused field, AmountInput's decimal-comma
* normalisation would rewrite the FIRST comma to a dot the moment the user touched it.
* "12,345" sats — a real amount someone is about to send — would become 12.345, which
* rounds to 12. Off by a factor of a thousand, silently, in the field that decides how
* much money leaves the wallet.
* The decimal-comma rewrite fires on whatever text the input hands back — and on
* Android a programmatically-set `value` is echoed straight back through onChangeText.
* So a grouped value in state gets exactly one keystroke, or one echo, before it means
* something else entirely.
*/
it('shows what NOT stripping on focus would have cost', () => {
const confirmed = '12,345'
it('shows what a grouped value in state would have become', () => {
const grouped = '1,000'
const ifLeftGrouped = normalizeDecimalComma(confirmed)
expect(ifLeftGrouped).toBe('12.345')
expect(toNumber(ifLeftGrouped)).toBe(12.345)
expect(Math.round(toNumber(ifLeftGrouped))).toBe(12) // vs 12345
expect(normalizeDecimalComma(grouped)).toBe('1.000')
expect(toNumber(normalizeDecimalComma(grouped))).toBe(1) // meant 1000
// Stripped first, the same keystroke path is harmless.
expect(normalizeDecimalComma(stripGrouping(confirmed))).toBe('12345')
expect(toNumber(normalizeDecimalComma(stripGrouping(confirmed)))).toBe(12345)
const bigger = '12,345'
expect(toNumber(normalizeDecimalComma(bigger))).toBe(12.345) // meant 12345
})
// A decimal-comma keyboard must still work: that is what the normalisation is FOR.
it('leaves the decimal-comma keyboard working', () => {
// Which is exactly why the rewrite is harmless against a plain value: there is no
// grouping comma for it to find, only a decimal one the user typed.
it('leaves a plain value — and the decimal-comma keyboard — untouched', () => {
expect(normalizeDecimalComma('1000')).toBe('1000')
expect(toNumber(normalizeDecimalComma('1000'))).toBe(1000)
// The rewrite's actual purpose.
expect(toNumber(normalizeDecimalComma('1,5'))).toBe(1.5)
})
})
describe('toNumber reads a grouped amount, whatever the source', () => {
// Parents pre-fill with grouped strings and AmountInput strips them on ingest, but the
// screens also parse their own amount state — so this has to hold either way.
it('parses grouped and plain alike', () => {
expect(toNumber('12,345')).toBe(12345)
expect(toNumber('12345')).toBe(12345)
expect(toNumber('1,234,567')).toBe(1234567)
expect(toNumber('1,234.56')).toBe(1234.56)
})
})

110
src/components/AmountInput.tsx
View File

@@ -74,33 +74,36 @@ export const AmountInput = forwardRef<TextInput, AmountInputProps>(
// --- grouping separators ---
//
// A confirmed amount is displayed grouped ("12,345"), but it must be a PLAIN number
// while it is being edited. `handleTopChange` rewrites the first comma to a dot, so a
// decimal-comma keyboard types "1,5" and means 1.5 — which also means that if a
// grouped value were left in the field, editing "12,345" would silently reinterpret it
// as 12.345. Stripping on focus and grouping again on confirm keeps the two meanings of
// "," from ever meeting.
// A confirmed amount is SHOWN grouped ("12,345"), but state always holds the plain
// number. Formatting is a render concern, derived from the value, never written back
// into it.
//
// That split is not tidiness, it is the fix. These are controlled TextInputs, and
// Android echoes a programmatically-set `value` back out through `onChangeText` —
// where `handleTopChange` rewrites the first comma to a dot, because a decimal-comma
// keyboard types "1,5" and means 1.5. Store a grouped string and that echo turns
// "1,000" into "1.000" and then, re-formatted, into something shorter still. Keep the
// grouping out of state and the round trip cannot happen: what the change handler sees
// is always what the user typed.
const stripGrouping = (v: string) => v.replace(/,/g, '')
// The 9-character cap is a limit on what may be TYPED, not on what may be shown:
// grouped, 9 digits becomes "999,999,999" (11 chars, or 14 with a fiat mantissa), and
// Android's maxLength filter truncates programmatically-set text too — it would render
// a confirmed amount as "999,999,9". The field is always unformatted while it has
// focus, so capping only then enforces exactly the same digit limit as before.
const typingMaxLength = 9
const displayMaxLength = 15
/** Group and pad a confirmed amount to the unit's precision: 12345 -> "12,345". */
const formatAmount = (v: string) => {
/** Group and pad an amount for DISPLAY: 12345 -> "12,345". Never stored. */
const formatForDisplay = (v: string, mantissa: number) => {
const n = toNumber(stripGrouping(v))
if (n === undefined || !Number.isFinite(n)) return v
return formatNumber(n, getCurrency(unit).mantissa)
return formatNumber(n, mantissa)
}
// Cap on what may be TYPED. Applied in the change handlers rather than via maxLength,
// because maxLength also clips programmatically-set text on Android — and the grouped
// display is longer than the number it shows ("999,999,999" is 11 characters, or 14
// with a fiat mantissa), so a maxLength that fit the typing limit would truncate the
// very value it had just formatted.
const MAX_TYPED_LENGTH = 9
const handleTopFocus = () => {
setHasTopAmountFocusedOnce(true)
setFocused('top')
setTopValue(current => stripGrouping(current))
onFocus?.()
}
@@ -112,7 +115,6 @@ export const AmountInput = forwardRef<TextInput, AmountInputProps>(
const handleBottomFocus = () => {
setHasBottomAmountFocusedOnce(true)
setFocused('bottom')
setBottomValue(current => stripGrouping(current))
onFocus?.()
}
@@ -164,9 +166,12 @@ export const AmountInput = forwardRef<TextInput, AmountInputProps>(
}
// keep internal state in sync with external `value`
//
// Stripped on the way in: a parent may hand us an already-grouped string (several
// screens pre-fill with numbro's thousandSeparated), and state must stay plain.
useEffect(() => {
log.trace(`[AmountInput.useEffect:mount] setTopValue`, value)
setTopValue(value)
setTopValue(stripGrouping(value))
// only show if
// - user has set conversion currency in settings
@@ -181,7 +186,7 @@ export const AmountInput = forwardRef<TextInput, AmountInputProps>(
if (canShow) {
log.trace(`[AmountInput.useEffect:mount] recalcBottom`, value)
setBottomValue(recalcBottom(value)) // ✅ always compute bottom from current top
setBottomValue(stripGrouping(recalcBottom(value))) // ✅ always compute bottom from current top
} else {
setBottomValue("0")
}
@@ -189,29 +194,35 @@ export const AmountInput = forwardRef<TextInput, AmountInputProps>(
useEffect(() => {
log.trace(`[AmountInput.useEffect:value] setTopValue`, value)
setTopValue(value)
setTopValue(stripGrouping(value))
if(!hasBeenFirstTimeConverted && value && toNumber(value) > 0) {
setBottomValue(recalcBottom(value))
setBottomValue(stripGrouping(recalcBottom(value)))
setHasBeenFirstTimeConverted(true)
}
}, [value])
// input change handlers (bi-directional)
const handleTopChange = (text: string) => {
// "," -> "." so a decimal-comma keyboard can type "1,5" and mean 1.5. Safe only
// because state never holds a grouped value for this to collide with.
const normalized = text.replace(',', '.')
if (normalized.length > MAX_TYPED_LENGTH) return
setTopValue(normalized)
if (focused === "top") {
setBottomValue(recalcBottom(normalized))
setBottomValue(stripGrouping(recalcBottom(normalized)))
onChangeText?.(normalized) // parent receives "top" value (sat or fiat, as per `unit`)
}
}
const handleBottomChange = (text: string) => {
const normalized = text.replace(',', '.')
if (normalized.length > MAX_TYPED_LENGTH) return
setBottomValue(normalized)
if (focused === "bottom") {
const newTop = recalcTop(normalized)
const newTop = stripGrouping(recalcTop(normalized))
setTopValue(newTop)
onChangeText?.(newTop) // keep parent synced to "top" side
}
@@ -219,27 +230,34 @@ export const AmountInput = forwardRef<TextInput, AmountInputProps>(
/**
* The amount is confirmed (keyboard "done", or focus left the field): group it.
* The amount is confirmed (keyboard "done", or focus left the field).
*
* The parent is told about the formatted string, not just the local field, so that what
* it holds and what the user sees are the same text. Everything downstream parses with
* `toNumber`, which reads grouping separators, so no caller has to care.
* Nothing is reformatted here, and nothing is pushed back to the parent: losing focus
* is all it takes for the field to render grouped, and the parent keeps the plain
* number it has had all along. The only work is recomputing the converted value from
* the CONFIRMED top rather than from whatever the bottom field holds — the two can be
* a keystroke apart.
*/
const onAmountEndEditing = () => {
const formattedTop = formatAmount(topValue)
setTopValue(formattedTop)
// Recompute the converted value from the CONFIRMED top, not from whatever the bottom
// field happens to hold — the two can be a keystroke apart.
setBottomValue(recalcBottom(formattedTop))
if (formattedTop !== topValue) {
onChangeText?.(formattedTop)
}
setBottomValue(stripGrouping(recalcBottom(topValue)))
return onEndEditing?.()
}
const bottomCurrencyCode = topIsSat ? fiatCode : CurrencyCode.SAT
const currencySymbol = bottomCurrencyCode ? Currencies[bottomCurrencyCode]!.symbol : null
// Grouped only when the field is NOT being edited. This is the whole of the "format on
// confirm" behaviour: losing focus is what confirms an amount, and the grouped text is
// derived at render time, so it can never be read back in as input.
const topDisplayValue = hasTopAmountFocusedOnce
? topValue
: formatForDisplay(topValue, getCurrency(unit).mantissa)
const bottomMantissa = bottomCurrencyCode ? Currencies[bottomCurrencyCode]!.mantissa : 0
const bottomDisplayValue = hasBottomAmountFocusedOnce
? bottomValue
: formatForDisplay(bottomValue, bottomMantissa)
// --- animations (unchanged behavior) ---
const topScale = useSharedValue(1)
const bottomScale = useSharedValue(1)
@@ -298,18 +316,16 @@ export const AmountInput = forwardRef<TextInput, AmountInputProps>(
const animatedSymbolStyle = useAnimatedStyle(() => ({
transform: [{ scale: bottomScale.value }], // sync with bottom input
fontSize: spacing.extraSmall * bottomScale.value, // scale font size
marginRight: focused === 'bottom' ? spacing.medium + bottomValue.length * 4.5 : spacing.tiny,
marginRight: focused === 'bottom' ? spacing.medium + bottomDisplayValue.length * 4.5 : spacing.tiny,
}))
const bottomCurrencyCode = topIsSat ? fiatCode : CurrencyCode.SAT
const currencySymbol = bottomCurrencyCode ? Currencies[bottomCurrencyCode]!.symbol : null
return (
<>
{/* Top input */}
<AnimatedTextInput
ref={ref}
value={topValue}
value={topDisplayValue}
onChangeText={handleTopChange}
onEndEditing={onAmountEndEditing}
onFocus={handleTopFocus}
@@ -320,7 +336,6 @@ export const AmountInput = forwardRef<TextInput, AmountInputProps>(
animatedTopStyle,
{ color: focused === 'top' ? focusedInputColor : convertedAmountColor }
]}
maxLength={hasTopAmountFocusedOnce ? typingMaxLength : displayMaxLength}
keyboardType="decimal-pad"
returnKeyType="done"
selectTextOnFocus={!hasTopAmountFocusedOnce}
@@ -343,7 +358,7 @@ export const AmountInput = forwardRef<TextInput, AmountInputProps>(
</Animated.Text>
<AnimatedTextInput
value={bottomValue}
value={bottomDisplayValue}
onChangeText={handleBottomChange}
onEndEditing={onAmountEndEditing}
onFocus={handleBottomFocus}
@@ -355,7 +370,6 @@ export const AmountInput = forwardRef<TextInput, AmountInputProps>(
animatedBottomStyle,
{ color: focused === 'bottom' ? focusedInputColor : convertedAmountColor }
]}
maxLength={hasBottomAmountFocusedOnce ? typingMaxLength : displayMaxLength}
keyboardType="decimal-pad"
returnKeyType="done"
selectTextOnFocus={!hasBottomAmountFocusedOnce}