defaultProps no longer assigned. #551
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors multiple React components to destructure props with inline default values and remove static defaultProps; several components alias Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App
participant PayButton
participant PaymentAPI as Payment Service
App->>PayButton: Render({ amount: A, ... })
Note right of PayButton #DDEEDD: Params destructured\namount -> initialAmount (defaulted)
PayButton->>PayButton: Initialize state from initialAmount
User->>PayButton: Click / interact
PayButton->>PaymentAPI: Create/track payment
PaymentAPI-->>PayButton: Result / events
PayButton-->>App: onSuccess / onTransaction callbacks
sequenceDiagram
autonumber
participant App
participant PaymentDialog
participant UI as Dialog UI
App->>PaymentDialog: Render({ disabled, theme: themeProp, ... })
Note right of PaymentDialog #E8F4E8: Derive internalDisabled\nfrom disabled prop
PaymentDialog->>UI: Render controls using internalDisabled
App-->>PaymentDialog: disabled prop changes
PaymentDialog->>PaymentDialog: useEffect updates internalDisabled
UI-->>PaymentDialog: User actions (close / success)
PaymentDialog-->>App: onClose / onSuccess callbacks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
react/lib/components/BarChart/BarChart.tsx (1)
43-47: Clear the timeout and clamp width to [0, 100] (also handles negatives).Prevents setState on unmounted component and ensures valid CSS width.
useEffect(() => { - setTimeout(() => { - setBarWidth(value > 100 ? 100 : Math.round(value)); - }, 800); + const id = window.setTimeout(() => { + const clamped = Math.min(100, Math.max(0, Math.round(value))); + setBarWidth(clamped); + }, 800); + return () => window.clearTimeout(id); }, [value]);react/lib/components/Button/Button.tsx (1)
57-73: borderRadius can becomeundefinedfor unsupported sizes. Provide a safe fallback.- const borderRadius = radiusBySize[(size ?? 'default') as keyof typeof radiusBySize]; + const borderRadius = + radiusBySize[(size ?? 'md') as keyof typeof radiusBySize] ?? '10px';react/lib/components/Widget/Widget.tsx (3)
305-349: Defaults added in signature butWidgetPropsstill requires these props. Make them optional to match runtime behavior.export interface WidgetProps { to: string; isChild?: boolean; @@ - success: boolean; + success?: boolean; successText?: string; theme?: ThemeName | Theme; foot?: React.ReactNode; - disabled: boolean; + disabled?: boolean; @@ - setNewTxs: Function; + setNewTxs?: (txs: Transaction[]) => void;
509-516: Also disconnect the Chronik socket on unmount to avoid leaks.return () => { if (thisAltpaymentSocket !== undefined) { thisAltpaymentSocket.disconnect(); setThisAltpaymentSocket(undefined); } + if (thisTxsSocket !== undefined) { + thisTxsSocket.disconnect(); + setThisTxsSocket(undefined); + } }
941-951:maxlengthshould bemaxLength(number). Current prop has no effect.- inputProps={{ maxlength: '12' }} + inputProps={{ maxLength: 12 }}react/lib/components/PayButton/PayButton.tsx (2)
57-59: Type conflict: You redeclaresizeandsizeScaleAlreadyAppliedas required, but they are optional inButtonProps. TS will error on the extension.export interface PayButtonProps extends ButtonProps { @@ - size: ButtonSize; - sizeScaleAlreadyApplied: boolean; + // Inherit optionality from ButtonProps; do not redeclare as required + // size?: ButtonSize; + // sizeScaleAlreadyApplied?: boolean;
235-247: Allow amount 0; current truthy check skips initialization for zero.- if (dialogOpen === false && initialAmount && currency) { + if (dialogOpen === false && initialAmount !== undefined && initialAmount !== null && currency) { const obj = getCurrencyObject( - Number(initialAmount), + Number(initialAmount), currency, randomSatoshis, );react/lib/components/PaymentDialog/PaymentDialog.tsx (2)
127-157: Fix invalid-amount oversight and first-render flicker; compute disabled consistently
- If to is valid but amount is invalid, internalDisabled can incorrectly be false. Also, initial state is false until the first effect tick.
Apply:
- const [internalDisabled, setInternalDisabled] = useState(false); + const [internalDisabled, setInternalDisabled] = useState(() => { + const hasValidAddress = + isValidCashAddress(to) || isValidXecAddress(to); + const invalidAmount = + amount !== undefined && isNaN(+amount); + return !hasValidAddress || invalidAmount || isPropsTrue(disabled); + }); @@ - useEffect(() => { - const invalidAmount = amount !== undefined && isNaN(+amount); - - if (to !== undefined && (isValidCashAddress(to) || isValidXecAddress(to))) { - setInternalDisabled(isPropsTrue(disabled)); - } else if (invalidAmount) { - setInternalDisabled(true); - } else { - setInternalDisabled(true); - } - }, [to, amount, disabled]); + useEffect(() => { + const hasValidAddress = + isValidCashAddress(to) || isValidXecAddress(to); + const invalidAmount = + amount !== undefined && isNaN(+amount); + setInternalDisabled( + !hasValidAddress || invalidAmount || isPropsTrue(disabled), + ); + }, [to, amount, disabled]);
235-241: Don’t disable the “Close” buttoninternalDisabled governs payment interactivity, not the ability to dismiss. Keep Close actionable.
- disabled={internalDisabled} /> + />
🧹 Nitpick comments (6)
react/lib/components/Button/Button.tsx (2)
126-135: TypeonClickto accept the MouseEvent (matches MUI) for safer interop.export interface ButtonProps { animation?: animation; text?: string; hoverText?: string; theme?: ThemeName | Theme; disabled?: boolean; - onClick?: () => void; + onClick?: React.MouseEventHandler<HTMLButtonElement>; size?: ButtonSize; sizeScaleAlreadyApplied?: boolean; }
184-185: Avoid a<div>inside<span>; use a non‑breaking space instead.- <span> {transitioning !== hovering ? hoverText : (text && text.trim() !== "" ? text : <div> </div>)} + <span> {transitioning !== hovering ? hoverText : (text && text.trim() !== "" ? text : '\u00A0')}react/lib/components/Widget/Widget.tsx (2)
732-751: Dependency list includes an unused outercurrency; remove to avoid needless re-creation.- }, [disabled, to, currency, opReturn]); + }, [disabled, to, opReturn]);
615-621: No-op state set.
setThisAddressType(thisAddressType)sets state to the same value every render of this effect; drop it.- setThisAddressType(thisAddressType); -react/lib/components/PayButton/PayButton.tsx (2)
284-294: Consider passinganimationdirectly toButtoninstead of wrapping a new component each render.Minor perf/readability win.
- const ButtonComponent: React.FC<ButtonProps> = ( - props: ButtonProps, - ): React.ReactElement => <Button animation={animation} {...props} />; - return ( <ThemeProvider value={theme}> - <ButtonComponent + <Button + animation={animation} onClick={handleButtonClick} text={text} hoverText={hoverText} disabled={disabled} size={size} sizeScaleAlreadyApplied={sizeScaleAlreadyApplied} />
226-232: Also clean uptxsSocketon dialog close/unmount.return () => { if (altpaymentSocket !== undefined) { altpaymentSocket.disconnect(); setAltpaymentSocket(undefined); } + if (txsSocket !== undefined) { + txsSocket.disconnect(); + setTxsSocket(undefined); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
paybutton/yarn.lockis excluded by!**/yarn.lock,!**/*.lockreact/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
react/lib/components/BarChart/BarChart.tsx(1 hunks)react/lib/components/Button/Button.tsx(2 hunks)react/lib/components/PayButton/PayButton.tsx(7 hunks)react/lib/components/PaymentDialog/PaymentDialog.tsx(4 hunks)react/lib/components/Widget/Widget.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
react/lib/components/Button/Button.tsx (1)
react/lib/themes/index.tsx (1)
useTheme(75-90)
react/lib/components/PayButton/PayButton.tsx (4)
react/lib/util/format.ts (2)
amount(5-12)isPropsTrue(66-75)react/lib/util/satoshis.ts (1)
getCurrencyObject(8-41)react/lib/themes/index.tsx (1)
useTheme(75-90)react/lib/util/address.ts (1)
isValidXecAddress(15-24)
react/lib/components/Widget/Widget.tsx (2)
react/lib/components/Button/Button.tsx (1)
Button(126-189)react/lib/util/address.ts (1)
getCurrencyTypeFromAddress(26-34)
react/lib/components/PaymentDialog/PaymentDialog.tsx (4)
react/lib/util/format.ts (1)
isPropsTrue(66-75)react/lib/components/Button/Button.tsx (3)
ButtonProps(10-19)Button(126-189)animation(8-8)react/lib/themes/index.tsx (1)
useTheme(75-90)react/lib/util/address.ts (1)
isValidXecAddress(15-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run build
🔇 Additional comments (2)
react/lib/components/PaymentDialog/PaymentDialog.tsx (2)
163-163: LGTM: theme alias + XEC detectionPassing isValidXecAddress(to) into useTheme is clean and keeps theme selection consistent with address type.
68-125: Destructure and forward disableScrollLock; confirm dialogOpen default
- Add disableScrollLock to the PaymentDialog props and forward it to the Dialog render as disableScrollLock={disableScrollLock} so consumer intent isn’t overridden.
- Verify whether dialogOpen = true here matches the previous defaultProps; restore the prior default if this is a breaking UX change.
File: react/lib/components/PaymentDialog/PaymentDialog.tsx (params block + Dialog render)
| amount: initialAmount, | ||
| opReturn, | ||
| disablePaymentId, | ||
| currency = '' as Currency, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid '' as Currency; default to the address-derived crypto.
- currency = '' as Currency,
+ currency = getCurrencyTypeFromAddress(to),Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In react/lib/components/PayButton/PayButton.tsx around line 66, the prop default
"currency = '' as Currency" should be removed and replaced by defaulting to the
address-derived crypto; determine the currency from the provided address (e.g.,
call the existing utility that maps address -> Currency or implement a small
helper like getCurrencyFromAddress(address)) and use that derived value as the
default when currency is undefined, or make currency optional and resolve it
inside the component by computing derivedCurrency = currency ??
getCurrencyFromAddress(address) before use.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
react/lib/components/BarChart/BarChart.tsx (1)
4-14: Fix duplicate interface declaration breaking the build (TS2687/TS2717).Consolidate to a single exported BarChartProps in react/lib/components/BarChart/BarChart.tsx (duplicate declarations present around lines 4–14): remove the first (required) declaration and keep the optional-properties version.
-export interface BarChartProps { - value: number; - color: string; - disabled: boolean; -} - -export interface BarChartProps { - value?: number; - color?: string; - disabled?: boolean; -} +export interface BarChartProps { + value?: number; + color?: string; + disabled?: boolean; +}
🧹 Nitpick comments (4)
react/lib/components/BarChart/BarChart.tsx (4)
49-53: Clear the timeout and clamp the value to [0, 100] to avoid stale updates and negative widths.Without cleanup, a late timeout can fire after unmount; also negative values render invalid widths.
- useEffect(() => { - setTimeout(() => { - setBarWidth(value > 100 ? 100 : Math.round(value)); - }, 800); - }, [value]); + useEffect(() => { + const timer = setTimeout(() => { + const clamped = Math.max(0, Math.min(100, Math.round(value))); + setBarWidth(clamped); + }, 800); + return () => clearTimeout(timer); + }, [value]);
31-37: Remove redundant type assertion.
barHolderis already typed asCSSProperties; theas React.CSSPropertiescast is unnecessary.- } as React.CSSProperties; + };
44-45: Minor: avoid unnecessary template literal.
backgroundColor: coloris equivalent and clearer.- backgroundColor: `${color}`, + backgroundColor: color,
24-25: Optional: simplify boolean check.Since
disabledis typed boolean with a default,disabled ? { filter: 'blur(5px)' } : {}is sufficient unless you intentionally accept stringy booleans from JS consumers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react/lib/components/BarChart/BarChart.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build test
react/lib/components/BarChart/BarChart.tsx
[error] 5-5: TS2687: All declarations of 'value' must have identical modifiers. Command: 'tsc --p ./tsconfig-build.json && microbundle --jsx React.createElement --format modern,cjs'.
[error] 6-6: TS2687: All declarations of 'color' must have identical modifiers. Command: 'tsc --p ./tsconfig-build.json && microbundle --jsx React.createElement --format modern,cjs'.
[error] 7-7: TS2687: All declarations of 'disabled' must have identical modifiers. Command: 'tsc --p ./tsconfig-build.json && microbundle --jsx React.createElement --format modern,cjs'.
[error] 11-11: TS2687: All declarations of 'value' must have identical modifiers. Command: 'tsc --p ./tsconfig-build.json && microbundle --jsx React.createElement --format modern,cjs'.
[error] 12-12: TS2687: All declarations of 'color' must have identical modifiers. Command: 'tsc --p ./tsconfig-build.json && microbundle --jsx React.createElement --format modern,cjs'.
[error] 13-13: TS2687: All declarations of 'disabled' must have identical modifiers. Command: 'tsc --p ./tsconfig-build.json && microbundle --jsx React.createElement --format modern,cjs'.
[error] 11-11: TS2717: Subsequent property declarations must have the same type. Property 'value' must be of type 'number', but here has type 'number | undefined'. Command: 'tsc --p ./tsconfig-build.json && microbundle --jsx React.createElement --format modern,cjs'.
[error] 12-12: TS2717: Subsequent property declarations must have the same type. Property 'color' must be of type 'string', but here has type 'string | undefined'. Command: 'tsc --p ./tsconfig-build.json && microbundle --jsx React.createElement --format modern,cjs'.
[error] 13-13: TS2717: Subsequent property declarations must have the same type. Property 'disabled' must be of type 'boolean', but here has type 'boolean | undefined'. Command: 'tsc --p ./tsconfig-build.json && microbundle --jsx React.createElement --format modern,cjs'.
🔇 Additional comments (1)
react/lib/components/BarChart/BarChart.tsx (1)
16-20: Props inline defaults LGTM.Destructured defaults align with optional props and remove the need for
defaultProps.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
react/lib/components/PayButton/PayButton.tsx (2)
66-66: Type-unsafe currency default; derive from address instead.The
currency = '' as Currencydefault bypasses type safety by casting an empty string. This is flagged in a previous review. UsegetCurrencyTypeFromAddress(to)to derive a valid default currency from the provided address.Based on past review comments.
Apply this diff:
- currency = '' as Currency, + currency = getCurrencyTypeFromAddress(to),
66-66: Avoid'' as Currency; default to address-derived crypto.The empty string is not a valid Currency and will cause issues downstream. The component already uses
getCurrencyTypeFromAddressat line 117. Apply the same logic here.Apply this diff:
- currency = '' as Currency, + currency = getCurrencyTypeFromAddress(to),
🧹 Nitpick comments (3)
react/lib/components/PaymentDialog/PaymentDialog.tsx (3)
127-127: Consider deriving disabled state instead of duplicating it.
internalDisabledis synced from thedisabledprop via useEffect. For simpler logic, consider computing it directly viauseMemoor inline:const internalDisabled = useMemo(() => { const invalidAmount = amount !== undefined && isNaN(+amount); if (to !== undefined && (isValidCashAddress(to) || isValidXecAddress(to))) { return isPropsTrue(disabled); } return invalidAmount || true; }, [to, amount, disabled]);This eliminates the extra state and synchronization.
Also applies to: 162-172
98-98: AlignautoClosedefault betweenPaymentDialogandPayButton.
PaymentDialogdefaultsautoClose = true(PaymentDialog.tsx:98), butPayButtondefaults it tofalse(PayButton.tsx:86) and always forwards that value. This inconsistency can lead to unexpected dialog-closing behavior whenPaymentDialogis used standalone vs. viaPayButton. Choose the intended default and update the other to match.
123-123: Consider explicit default fordisabledprop.While
isPropsTrue(disabled)handles undefined correctly (returns false), an explicit default would improve clarity and align with the pattern used in Button and PayButton.Apply this diff:
- disabled, + disabled = false,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react/lib/components/PayButton/PayButton.tsx(7 hunks)react/lib/components/PaymentDialog/PaymentDialog.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
react/lib/components/PaymentDialog/PaymentDialog.tsx (4)
react/lib/util/format.ts (1)
isPropsTrue(66-75)react/lib/components/Button/Button.tsx (3)
ButtonProps(10-19)Button(126-189)animation(8-8)react/lib/themes/index.tsx (1)
useTheme(75-90)react/lib/util/address.ts (1)
isValidXecAddress(15-24)
react/lib/components/PayButton/PayButton.tsx (5)
react/lib/util/types.ts (1)
Currency(140-140)react/lib/util/format.ts (2)
amount(5-12)isPropsTrue(66-75)react/lib/util/satoshis.ts (1)
getCurrencyObject(8-41)react/lib/themes/index.tsx (1)
useTheme(75-90)react/lib/util/address.ts (1)
isValidXecAddress(15-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run build
🔇 Additional comments (11)
react/lib/components/PayButton/PayButton.tsx (6)
95-95: LGTM! Clean state initialization and prop sync.The amount state is correctly seeded from
initialAmountand kept in sync via useEffect when the prop changes.Also applies to: 152-154
157-169: LGTM! Validation logic correctly updated for renamed props.All references to
initialAmountanddisabledPropare consistent with the destructured parameter names, and dependencies are correctly updated.
235-246: LGTM! Currency object creation correctly usesinitialAmount.The useEffect properly references the renamed
initialAmountprop throughout, and dependencies are correctly updated.
278-278: LGTM! Theme aliasing correctly applied.The
themePropalias is properly passed touseTheme, consistent with the destructured parameter naming.
61-91: Inline defaults migration looks correct.The refactoring successfully moves defaults from static
defaultPropsto inline destructuring. Prop aliasing (initialAmount,disabledProp,themeProp) avoids shadowing internal state and is consistently applied throughout the component.
152-154: Amount synchronization is correct.The effect properly syncs internal
amountstate when theinitialAmountprop changes, with the correct dependency array.react/lib/components/PaymentDialog/PaymentDialog.tsx (5)
178-178: LGTM! Theme aliasing correctly applied.The
themePropalias is properly passed touseTheme, consistent with the destructured parameter naming.
218-218: LGTM!internalDisabledcorrectly propagated to child components.Both
WidgetContainerandButtonComponentreceive theinternalDisabledstate, maintaining consistent disabled behavior.Also applies to: 255-255
127-172: Internal disabled state management is correct.The rename to
internalDisabledclearly distinguishes component-managed state from the externaldisabledprop. The validation logic properly forces disabled state on invalid inputs while respecting the external prop when inputs are valid.
178-178: Theme resolution is correct.Theme aliasing to
themePropand usage withuseThemeis consistent with the pattern used in PayButton and Button components.
68-125: Inline defaults migration successfully implemented.The refactoring removes static
defaultPropsin favor of inline destructuring with defaults, aligning PaymentDialog with the broader migration pattern across the codebase.
Description
Fixes this warning on 3rd party apps:
Test plan
Requires top-to-bottom testing, paying close attention to default config option values.
Summary by CodeRabbit
Refactor
Improvements