Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat(suite): Enhance HiddenPlaceholder #13914

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/react-utils/jest.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
const baseConfig = require('../../jest.config.base');

module.exports = {
...baseConfig,
collectCoverage: true,
collectCoverageFrom: ['src/**/*.ts'],
testEnvironment: '../../JestCustomEnv.js',
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

react-utils did not have unit tests at all. I set it up for the first test. Code coverage is not great 😄

1 change: 1 addition & 0 deletions packages/react-utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"main": "src/index",
"scripts": {
"lint:js": "yarn g:eslint '**/*.{ts,tsx,js}'",
"test:unit": "yarn g:jest --verbose -c ./jest.config.js",
"depcheck": "yarn g:depcheck",
"type-check": "yarn g:tsc --build"
},
Expand Down
1 change: 1 addition & 0 deletions packages/react-utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ export { useOnClickOutside } from './hooks/useOnClickOutside';
export { useTimer } from './hooks/useTimer';
export { useWindowFocus } from './hooks/useWindowFocus';
export type { Timer } from './hooks/useTimer';
export { rewriteReactNodeRecursively } from './rewriteReactNodeRecursively';
37 changes: 37 additions & 0 deletions packages/react-utils/src/rewriteReactNodeRecursively.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { ReactNode, isValidElement, cloneElement, Key } from 'react';

type RewriteReactNodeRecursivelyCallback = (stringSubNode: string) => string;

/**
* Recursively crawls ReactNode and applies callback on all string subnodes
* @param node ReactNode to be recursively crawled & modified
* @param callback function to be applied on all string subnodes
* @param reactKey optional React key applied on the root node, if it is a ReactElement. Used internally.
* @returns modified ReactNode
*/
export const rewriteReactNodeRecursively = (
node: ReactNode,
callback: RewriteReactNodeRecursivelyCallback,
reactKey?: Key,
): ReactNode => {
if (typeof node === 'string') return callback(node);
if (typeof node === 'number') return callback(node.toString());

if (Array.isArray(node)) {
return node.map((child, index) => {
const newReactKey: Key | undefined = isValidElement(child)
? child.key ?? index
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When rewriting the ReactNode, I do have to provide key even when the original element did not.
This is not an edge case, it's actually very common:
<span>label<i>123</i></span>
the span has children = array ["label", <i>123</i>]. Now the <i> has to have a key, idk why 🤦

: undefined;

return rewriteReactNodeRecursively(child, callback, newReactKey);
});
}

if (isValidElement(node)) {
const rewrittenChildren = rewriteReactNodeRecursively(node.props.children, callback);

return cloneElement(node, { ...node.props, key: reactKey, children: rewrittenChildren });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TL;DR: Should I scrap this? 🤔

My idea was to have HiddenPlaceholder.tsx alone handle redacting numbers, so you would just wrap any content with it and it'd be completely automatic.
As I wrote in the comment there, this only works for simple cases of components wrapping children, such as:
<Wrapper>Label: <SomeStyle>12345 CZK</SomeStyle></Wrapper>.

But this procedure simply stops when it encounters more complex components, which calculate their content instead of just wrapping children, like <Formatter value={1234} />. They're returned "as is".

So those components have to do the redacting themselves: see prepareFiatAmountFormatter.ts, prepareCryptoAmountFormatter.ts.

I'm not happy with this mixed approach, where sometimes it does things automagically under the hood, but sometimes you just have to do it yourself. I fear this might lead to confusion and mistakes later on.
So should we drop this magic, and redact numbers at all specific places where we display them?
Reminder: this won't make HiddenPlaceholder.tsx useless, it still does CSS blur, and will provide the context for shouldRedactNumbers.

}

return node;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`rewriteReactNodeRecursively should recursively rewrite string subnodes in a ReactElement 1`] = `
<span>
This is barbar,
<i>
it has a bar
</i>
.
</span>
`;
55 changes: 55 additions & 0 deletions packages/react-utils/tests/rewriteReactNodeRecursively.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { rewriteReactNodeRecursively } from '../src/rewriteReactNodeRecursively';

describe('rewriteReactNodeRecursively', () => {
const callback = (stringSubNode: string) => stringSubNode.replace(/foo/g, 'bar');

it('should rewrite a string subnode', () => {
const inputNode = 'This is foobar.';
const expectedNode = 'This is barbar.';
expect(rewriteReactNodeRecursively(inputNode, callback)).toBe(expectedNode);
});

it('should cast a number subnode to string and rewrite it', () => {
const callbackForNumbers = (stringifiedNumber: string) =>
stringifiedNumber.replace(/34/g, '***');
const inputNode = 123456;
const expectedNode = '12***56';
expect(rewriteReactNodeRecursively(inputNode, callbackForNumbers)).toBe(expectedNode);
});

it('should return the node if it is a primitive other than string or number', () => {
expect(rewriteReactNodeRecursively(null, callback)).toBe(null);
expect(rewriteReactNodeRecursively(undefined, callback)).toBe(undefined);
expect(rewriteReactNodeRecursively(true, callback)).toBe(true);
});

it('should rewrite all string subnodes in an array', () => {
const inputNode = ['This is foobar.', 'It has a foo.'];
const expectedNode = ['This is barbar.', 'It has a bar.'];
expect(rewriteReactNodeRecursively(inputNode, callback)).toEqual(expectedNode);
});

it('should recursively rewrite string subnodes in a ReactElement', () => {
const inputNode = (
<span>
This is foobar, <i>it has a foo</i>.
</span>
);
expect(rewriteReactNodeRecursively(inputNode, callback)).toMatchSnapshot();
});

it('should iterate through a collection of ReactNodes, preserving keys or setting them to index if necessary', () => {
const inputNode = [
// eslint-disable-next-line react/jsx-key
<span>Foo does not have key!.</span>,
<span key="id234">This is foo with key.</span>,
false,
] as const;

const result = rewriteReactNodeRecursively(inputNode, callback) as typeof inputNode;

expect(result.length).toBe(3);
expect(result[0].key).toBe('0');
expect(result[1].key).toBe('id234');
});
});
30 changes: 26 additions & 4 deletions packages/suite/src/components/suite/HiddenPlaceholder.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { ReactNode, useEffect, useRef, useState } from 'react';
import { ReactNode, useEffect, useMemo, useRef, useState } from 'react';
import styled, { css } from 'styled-components';
import { useSelector } from 'src/hooks/suite';
import { selectIsDiscreteModeActive } from 'src/reducers/wallet/settingsReducer';
import { rewriteReactNodeRecursively } from '@trezor/react-utils';
import { redactNumericalSubstring, RedactNumbersContext } from '@suite-common/wallet-utils';

interface WrapperProps {
$intensity: number;
Expand Down Expand Up @@ -34,11 +36,13 @@ export const HiddenPlaceholder = ({
children,
enforceIntensity,
className,
...rest
'data-testid': dataTestId,
}: HiddenPlaceholderProps) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

incidental cleanup, rest did not contain anything else

const ref = useRef<HTMLSpanElement>(null);
const [automaticIntensity, setAutomaticIntensity] = useState(10);
const discreetMode = useSelector(selectIsDiscreteModeActive);
const [isHovered, setIsHovered] = useState(false);
const shouldRedactNumbers = discreetMode && !isHovered;

useEffect(() => {
if (ref.current === null) {
Expand All @@ -54,15 +58,33 @@ export const HiddenPlaceholder = ({
setAutomaticIntensity(fontSize / 5);
}, []);

/*
Recursively redact all numbers in children hierarchy of HiddenPlaceholder.
This works for children hierarchy that consists of simple components which wrap another 'children'.
When applied to complex components (Formatters), only the blur is applied from HiddenPlaceholder.
Redaction is handled in prepareFiatAmountFormatter, prepareCryptoAmountFormatter.
*/
const modifiedChildren = useMemo(
() =>
shouldRedactNumbers
? rewriteReactNodeRecursively(children, redactNumericalSubstring)
: children,
[children, shouldRedactNumbers],
);

return (
<Wrapper
onMouseEnter={() => setIsHovered(true)}
onMouseLeave={() => setIsHovered(false)}
$discreetMode={discreetMode}
$intensity={enforceIntensity !== undefined ? enforceIntensity : automaticIntensity}
className={className}
ref={ref}
data-testid={rest['data-testid']}
data-testid={dataTestId}
>
{children}
<RedactNumbersContext.Provider value={{ shouldRedactNumbers }}>
{modifiedChildren}
</RedactNumbersContext.Provider>
</Wrapper>
);
};
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import { A } from '@mobily/ts-belt';

import { networks, NetworkSymbol } from '@suite-common/wallet-config';
import { amountToSatoshi, formatAmount } from '@suite-common/wallet-utils';
import {
amountToSatoshi,
formatAmount,
redactNumericalSubstring,
} from '@suite-common/wallet-utils';
import { PROTO } from '@trezor/connect';

import { makeFormatter } from '../makeFormatter';
Expand Down Expand Up @@ -46,6 +50,7 @@ export const prepareCryptoAmountFormatter = (config: FormatterConfig) =>
maxDisplayedDecimals = 8,
isEllipsisAppended = true,
},
shouldRedactNumbers,
) => {
const { bitcoinAmountUnit } = config;

Expand Down Expand Up @@ -84,10 +89,10 @@ export const prepareCryptoAmountFormatter = (config: FormatterConfig) =>
if (withSymbol) {
const NetworkSymbolFormatter = prepareNetworkSymbolFormatter(config);

return `${formattedValue} ${NetworkSymbolFormatter.format(symbol!)}`;
formattedValue = `${formattedValue} ${NetworkSymbolFormatter.format(symbol!)}`;
}

return formattedValue;
return shouldRedactNumbers ? redactNumericalSubstring(formattedValue) : formattedValue;
},
'CryptoAmountFormatter',
);
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { FormatNumberOptions } from '@formatjs/intl';

import { BigNumber } from '@trezor/utils/src/bigNumber';
import { redactNumericalSubstring } from '@suite-common/wallet-utils';

import { makeFormatter } from '../makeFormatter';
import { FormatterConfig } from '../types';
Expand All @@ -14,24 +15,30 @@ export const prepareFiatAmountFormatter = (config: FormatterConfig) =>
string | number,
string | null,
FiatAmountFormatterDataContext<FormatNumberOptions>
>((value, dataContext) => {
>((value, dataContext, shouldRedactNumbers) => {
const { intl, fiatCurrency } = config;

const { style, currency, minimumFractionDigits, maximumFractionDigits } = dataContext;
const fiatValue = new BigNumber(value);
const currencyForDisplay = currency ?? fiatCurrency;

if (fiatValue.isNaN()) {
return null;
}

let formattedValue: string = '';

if (fiatValue.gt(Number.MAX_SAFE_INTEGER)) {
return `${value} ${currency ?? fiatCurrency}`;
formattedValue = `${value} ${currencyForDisplay}`;
}

return intl.formatNumber(fiatValue.toNumber(), {
formattedValue = intl.formatNumber(fiatValue.toNumber(), {
...dataContext,
style: style || 'currency',
currency: currency ?? fiatCurrency,
currency: currencyForDisplay,
minimumFractionDigits: minimumFractionDigits ?? 2,
maximumFractionDigits: maximumFractionDigits ?? 2,
});

return shouldRedactNumbers ? redactNumericalSubstring(formattedValue) : formattedValue;
}, 'FiatAmountFormatter');
14 changes: 9 additions & 5 deletions suite-common/formatters/src/makeFormatter.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { useShouldRedactNumbers } from '@suite-common/wallet-utils';

export type DataContext = Record<string, unknown>;

interface FormatDefinition<TInput, TOutput, TDataContext extends DataContext> {
Expand All @@ -6,6 +8,8 @@ interface FormatDefinition<TInput, TOutput, TDataContext extends DataContext> {
value: TInput,
/** Additional data context to be used by the formatter. */
dataContext: Partial<TDataContext>,
/** Whether a component above has requested to redact the numbers for discreet mode */
shouldRedactNumbers?: boolean,
): TOutput;
}

Expand Down Expand Up @@ -40,11 +44,11 @@ export const makeFormatter = <TInput, TOutput, TDataContext extends DataContext
format: FormatDefinition<TInput, TOutput, TDataContext>,
displayName = 'Formatter',
): Formatter<TInput, TOutput, TDataContext> => {
const formatter: Formatter<TInput, TOutput, TDataContext> = props =>
<>{format(props.value, props)}</> ?? null;
formatter.displayName = displayName;
const FormatterComponent: Formatter<TInput, TOutput, TDataContext> = props =>
<>{format(props.value, props, useShouldRedactNumbers())}</> ?? null;
FormatterComponent.displayName = displayName;

formatter.format = (value, dataContext = {}) => format(value, dataContext);
FormatterComponent.format = (value, dataContext = {}) => format(value, dataContext);

return formatter;
return FormatterComponent;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is actually a React component, so its name should be in CamelCase.
Otherwise I get an unjustified error from eslint rule of hooks because of calling useShouldRedactNumbers.

};
29 changes: 29 additions & 0 deletions suite-common/wallet-utils/src/__tests__/discreetModeUtils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { redactNumericalSubstring } from '../discreetModeUtils';

const MAX_PLACEHOLDER = '####'; // placeholder with maximal length of 4 characters

describe('redactNumericalSubstring', () => {
it('replaces sole stringified number with placeholder', () => {
expect(redactNumericalSubstring('123')).toBe('###');
expect(redactNumericalSubstring('0.001234')).toBe(MAX_PLACEHOLDER);
expect(redactNumericalSubstring('-9,876,543.210001')).toBe(MAX_PLACEHOLDER);
expect(redactNumericalSubstring('-.1')).toBe('###');
});

it('redacts only the number, not an accompanying symboll', () => {
expect(redactNumericalSubstring('CZK 123')).toBe(`CZK ###`);
expect(redactNumericalSubstring('0.001234 BTC')).toBe(`${MAX_PLACEHOLDER} BTC`);
expect(redactNumericalSubstring('-9,876,543.210001€')).toBe(`${MAX_PLACEHOLDER}€`);
});

it('redacts all number occurrences', () => {
expect(redactNumericalSubstring('CZK 123 is 12345 satoshi')).toBe(
`CZK ### is ${MAX_PLACEHOLDER} satoshi`,
);
});

it('does not replace non-numerical strings', () => {
expect(redactNumericalSubstring('foo')).toBe('foo');
expect(redactNumericalSubstring('. , -.')).toBe('. , -.');
});
});
32 changes: 32 additions & 0 deletions suite-common/wallet-utils/src/discreetModeUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { createContext, useContext } from 'react';

const numericalSubstringRegex = /[\d\-\.,]*\d+[\.\d]*/g;
/**
* Search the input for a numerical substring and replace it with DISCREET_PLACEHOLDER.
* @param value whole string value, usually number with symbol
* @returns redacted string
*/
export const redactNumericalSubstring = (value: string): string => {
const PLACEHOLDER_CHAR = '#';
const MAX_PLACEHOLDER_LENGTH = 4;

const matches = value.match(numericalSubstringRegex);
if (!matches) return value;

return matches.reduce((redactedValue, match) => {
const newLength = Math.min(MAX_PLACEHOLDER_LENGTH, match.length);

return redactedValue.replace(match, PLACEHOLDER_CHAR.repeat(newLength));
}, value);
};

type RedactNumbersContextData = { shouldRedactNumbers: boolean };
export const RedactNumbersContext = createContext<RedactNumbersContextData>({
shouldRedactNumbers: false,
});
/**
* Determine whether we are under a component which currently requests to redact the numbers for discreet mode (see HiddenPlaceholder).
* @returns shouldRedactNumbers whether numbers should be redacted in displayed output
*/
export const useShouldRedactNumbers = () =>
useContext(RedactNumbersContext)?.shouldRedactNumbers ?? false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if all of this belongs to wallet-utils. Seemed suitable to me. Where would you place it?

1 change: 1 addition & 0 deletions suite-common/wallet-utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ export * from './backendUtils';
export * from './balanceUtils';
export * from './cardanoUtils';
export * from './csvParserUtils';
export * from './discreetModeUtils';
export * from './ethUtils';
export * from './fiatConverterUtils';
export * from './fiatRatesUtils';
Expand Down
Loading