From 007859ddf6a055870aab1a98415f62adfbdac248 Mon Sep 17 00:00:00 2001 From: Keshinro Tanitoluwa Joseph Date: Fri, 29 May 2026 12:52:03 +0100 Subject: [PATCH 1/2] chore: prevent component prop spreading --- docs/prop-patterns.md | 129 ++++++++++++++++++ src/components/common/AppText.tsx | 78 ++++++++++- src/components/mobile/AccessibleButton.tsx | 37 ++++- src/components/mobile/InfiniteVirtualList.tsx | 45 +++++- src/components/mobile/MobileFormInput.tsx | 69 +++++++++- src/components/mobile/VirtualList.tsx | 42 +++++- 6 files changed, 377 insertions(+), 23 deletions(-) create mode 100644 docs/prop-patterns.md diff --git a/docs/prop-patterns.md b/docs/prop-patterns.md new file mode 100644 index 00000000..96d32d6b --- /dev/null +++ b/docs/prop-patterns.md @@ -0,0 +1,129 @@ +# Component Prop Patterns + +This document explains the prop-passing conventions used in TeachLink mobile and why they exist. + +--- + +## The Problem with `{...props}` / `{...rest}` Spreading + +Spreading all props blindly onto a child element has several downsides: + +1. **Unnecessary re-renders** — any prop change in the bag, including ones the component doesn't use, triggers a re-render of the child. +2. **Hidden contracts** — callers can't tell from the interface which props are safe to pass without reading the implementation. +3. **Prop pollution** — non-standard props may silently land on native elements and produce React warnings. + +--- + +## Rule: Always Use Explicit Prop Destructuring in Wrapper Components + +### ✅ Do this + +```tsx +// Good: explicit contract, predictable rendering +interface MyButtonProps { + label: string; + onPress: () => void; + testID?: string; + disabled?: boolean; +} + +export const MyButton: React.FC = ({ + label, + onPress, + testID, + disabled, +}) => ( + + {label} + +); +``` + +### ❌ Don't do this + +```tsx +// Bad: opaque contract, unnecessary re-renders +interface MyButtonProps extends TouchableOpacityProps { + label: string; +} + +export const MyButton: React.FC = ({ label, ...rest }) => ( + + {label} + +); +``` + +--- + +## When Spreading Is Acceptable + +There are two legitimate exceptions to the above rule: + +### 1. Higher-Order Components (HOC) + +A HOC wraps an *unknown* component type and must forward **all** of its props. The spread is unavoidable here because the wrapped component's props are not known at compile time. + +```tsx +// AuthGuard.tsx — must forward every prop of the wrapped component +export function withAuthGuard

( + Component: React.ComponentType

, +): React.ComponentType

{ + return function AuthGuardedComponent(props: P) { + return ( + + + + ); + }; +} +``` + +Similarly, `LazyScreen.tsx` uses `{...props}` because it wraps a lazily-loaded, unknown component type. + +### 2. React Navigation Drawer Internals + +`MobileDrawer` must forward the entire `DrawerContentComponentProps` bag to `` and `` because React Navigation requires this exact shape for navigation to function. + +### 3. Computed Internal Objects + +Spreading a **locally-computed** object (not external props) is fine, because the object's shape is known and controlled: + +```tsx +// PullToRefresh.tsx — spreading internal PanResponder handlers + +``` + +These computed-object spreads are not the same as forwarding unknown external props. + +--- + +## Adding New Props to a Wrapper Component + +1. Add the new prop to the component's **explicit interface** with a JSDoc comment. +2. Destructure it from the function parameters. +3. Pass it by name to the underlying element. +4. Do **not** fall back to `...rest` — keep the list explicit. + +--- + +## Files That Were Refactored (Issue #371) + +| Component | Change | +|---|---| +| `AppText.tsx` | Replaced `{...props}` with explicit `TextProps` subset | +| `AccessibleButton.tsx` | Replaced `{...rest}` with explicit `TouchableOpacityProps` subset | +| `MobileFormInput.tsx` | Replaced `{...rest}` with explicit `TextInputProps` subset | +| `InfiniteVirtualList.tsx` | Replaced `{...rest}` with explicit `FlatListProps` extension | +| `VirtualList.tsx` | Replaced `{...rest}` with explicit `FlatListProps` extension | + +Files where spreading was **intentionally kept** (see exceptions above): + +| File | Reason | +|---|---| +| `AuthGuard.tsx` | HOC pattern — must forward all props of wrapped component | +| `LazyScreen.tsx` | Generic lazy-loader HOC — same as above | +| `MobileDrawer.tsx` | React Navigation requires full `DrawerContentComponentProps` forwarding | +| `PullToRefresh.tsx` | Internal computed objects (PanResponder handlers, scroll props) | +| `MobileVideoPlayer.tsx` | Internal computed PanResponder handlers | +| `AchievementBadges.tsx` | Computed accessibility object spread — not external prop forwarding | diff --git a/src/components/common/AppText.tsx b/src/components/common/AppText.tsx index abfca2fd..47da037a 100644 --- a/src/components/common/AppText.tsx +++ b/src/components/common/AppText.tsx @@ -1,20 +1,76 @@ import React from 'react'; -import { Text as RNText, TextProps, StyleSheet } from 'react-native'; +import { Text as RNText, StyleSheet } from 'react-native'; +import type { + StyleProp, + TextStyle, + AccessibilityRole, + AccessibilityState, +} from 'react-native'; import { useDynamicFontSize } from '../../hooks'; -interface AppTextProps extends TextProps { +/** + * Explicit subset of React Native TextProps that AppText consumers may pass. + * + * NOTE: Do NOT add `{...rest}` or generic prop spreading here. + * If you need an additional prop, add it explicitly to this interface + * and thread it through to below. This keeps the component + * surface area predictable and prevents unnecessary re-renders caused + * by unknown prop changes. See docs/prop-patterns.md. + */ +export interface AppTextProps { + /** Text content */ + children?: React.ReactNode; + /** Style overrides for the text element */ + style?: StyleProp; /** * If true, the font size will NOT be scaled. * Useful for elements that should remain at a fixed size regardless of system settings. */ fixed?: boolean; + /** Maximum number of lines before truncation */ + numberOfLines?: number; + /** Truncation strategy when numberOfLines is set */ + ellipsizeMode?: 'head' | 'middle' | 'tail' | 'clip'; + /** Press handler */ + onPress?: () => void; + /** Long-press handler */ + onLongPress?: () => void; + /** Test identifier for automated tests */ + testID?: string; + /** Accessibility label for screen readers */ + accessibilityLabel?: string; + /** Accessibility role for screen readers */ + accessibilityRole?: AccessibilityRole; + /** Accessibility hint for screen readers */ + accessibilityHint?: string; + /** Accessibility state for screen readers */ + accessibilityState?: AccessibilityState; + /** Controls importance for accessibility on Android */ + importantForAccessibility?: 'auto' | 'yes' | 'no' | 'no-hide-descendants'; + /** Allow the text to be selected by the user */ + selectable?: boolean; } /** * A wrapper around React Native's Text component that uses the useDynamicFontSize hook * to ensure consistent scaling across the application. */ -export const AppText: React.FC = ({ style, fixed = false, ...props }) => { +export const AppText: React.FC = ({ + style, + fixed = false, + children, + numberOfLines, + ellipsizeMode, + onPress, + onLongPress, + testID, + accessibilityLabel, + accessibilityRole, + accessibilityHint, + accessibilityState, + importantForAccessibility, + selectable, +}) => { const { scale } = useDynamicFontSize(); // We flatten the style to easily extract and modify the fontSize @@ -33,11 +89,23 @@ export const AppText: React.FC = ({ style, fixed = false, ...props return ( + numberOfLines={numberOfLines} + ellipsizeMode={ellipsizeMode} + onPress={onPress} + onLongPress={onLongPress} + testID={testID} + accessibilityLabel={accessibilityLabel} + accessibilityRole={accessibilityRole} + accessibilityHint={accessibilityHint} + accessibilityState={accessibilityState} + importantForAccessibility={importantForAccessibility} + selectable={selectable} + > + {children} + ); }; diff --git a/src/components/mobile/AccessibleButton.tsx b/src/components/mobile/AccessibleButton.tsx index c113908c..1fbb5459 100644 --- a/src/components/mobile/AccessibleButton.tsx +++ b/src/components/mobile/AccessibleButton.tsx @@ -1,17 +1,22 @@ import React from 'react'; import { TouchableOpacity, - TouchableOpacityProps, StyleSheet, ViewStyle, StyleProp, + GestureResponderEvent, } from 'react-native'; import { getAccessibilityProps } from '../../utils/accessibility'; /** - * Props for the AccessibleButton component + * Explicit props for the AccessibleButton component. + * + * NOTE: Do NOT reintroduce `{...rest}` or generic prop spreading here. + * If you need an additional TouchableOpacity prop, add it explicitly to this + * interface and thread it through to below. + * See docs/prop-patterns.md. */ -interface AccessibleButtonProps extends TouchableOpacityProps { +interface AccessibleButtonProps { /** Accessibility label for screen readers */ label: string; /** Additional accessibility hint for screen readers */ @@ -20,6 +25,20 @@ interface AccessibleButtonProps extends TouchableOpacityProps { role?: 'button' | 'link'; /** Optional custom styles for the button container */ containerStyle?: StyleProp; + /** Style applied to the TouchableOpacity */ + style?: StyleProp; + /** Button content */ + children?: React.ReactNode; + /** Whether the button is disabled */ + disabled?: boolean; + /** Opacity when pressed. Defaults to 0.7 */ + activeOpacity?: number; + /** Press handler */ + onPress?: (event: GestureResponderEvent) => void; + /** Long-press handler */ + onLongPress?: (event: GestureResponderEvent) => void; + /** Test identifier for automated tests */ + testID?: string; } /** @@ -35,19 +54,23 @@ export const AccessibleButton: React.FC = ({ containerStyle, disabled, activeOpacity = 0.7, - ...rest -}: AccessibleButtonProps) => { + onPress, + onLongPress, + testID, +}) => { const accessibilityProps = getAccessibilityProps(label, role as 'button' | 'link', hint, { disabled: !!disabled, }); return ( {children} diff --git a/src/components/mobile/InfiniteVirtualList.tsx b/src/components/mobile/InfiniteVirtualList.tsx index 80ecfd71..28858ef5 100644 --- a/src/components/mobile/InfiniteVirtualList.tsx +++ b/src/components/mobile/InfiniteVirtualList.tsx @@ -12,6 +12,13 @@ import { import * as Device from 'expo-device'; import { useMemoryMonitor } from '../../hooks'; +/** + * Explicit extension props for InfiniteVirtualList. + * + * NOTE: Do NOT reintroduce `{...rest}` or generic prop spreading here. + * All supported FlatList extension points must be listed explicitly. + * See docs/prop-patterns.md. + */ export interface InfiniteVirtualListProps extends Omit, 'renderItem'> { /** The data items to display. */ data: ReadonlyArray | null | undefined; @@ -33,6 +40,24 @@ export interface InfiniteVirtualListProps extends Omit, 'ren style?: StyleProp; /** Custom styles for the inner scroll container. */ contentContainerStyle?: StyleProp; + /** Component rendered above the list items. */ + ListHeaderComponent?: FlatListProps['ListHeaderComponent']; + /** Component rendered when the list is empty. */ + ListEmptyComponent?: FlatListProps['ListEmptyComponent']; + /** Render horizontally instead of vertically. */ + horizontal?: boolean; + /** Hide the vertical scroll indicator. */ + showsVerticalScrollIndicator?: boolean; + /** Hide the horizontal scroll indicator. */ + showsHorizontalScrollIndicator?: boolean; + /** Pull-to-refresh control. */ + refreshControl?: FlatListProps['refreshControl']; + /** Scroll event throttle in milliseconds. */ + scrollEventThrottle?: number; + /** Scroll event callback. */ + onScroll?: FlatListProps['onScroll']; + /** Test identifier for automated tests. */ + testID?: string; } /** @@ -51,7 +76,15 @@ export function InfiniteVirtualList({ listId = 'InfiniteVirtualList', style, contentContainerStyle, - ...rest + ListHeaderComponent, + ListEmptyComponent, + horizontal, + showsVerticalScrollIndicator, + showsHorizontalScrollIndicator, + refreshControl, + scrollEventThrottle, + onScroll, + testID, }: InfiniteVirtualListProps) { // Monitor JS collection array sizes useMemoryMonitor({ @@ -112,12 +145,20 @@ export function InfiniteVirtualList({ onEndReached={onEndReached} onEndReachedThreshold={onEndReachedThreshold} ListFooterComponent={renderFooter} + ListHeaderComponent={ListHeaderComponent} + ListEmptyComponent={ListEmptyComponent} // Performance configurations: removeClippedSubviews={true} // Free native memory by unmounting offscreen views style={style} contentContainerStyle={contentContainerStyle} + horizontal={horizontal} + showsVerticalScrollIndicator={showsVerticalScrollIndicator} + showsHorizontalScrollIndicator={showsHorizontalScrollIndicator} + refreshControl={refreshControl} + scrollEventThrottle={scrollEventThrottle} + onScroll={onScroll} + testID={testID} {...optimizations} - {...rest} /> ); } diff --git a/src/components/mobile/MobileFormInput.tsx b/src/components/mobile/MobileFormInput.tsx index f47a5317..b5605bcd 100644 --- a/src/components/mobile/MobileFormInput.tsx +++ b/src/components/mobile/MobileFormInput.tsx @@ -1,19 +1,36 @@ import React, { useState } from 'react'; -import { View, TextInput, TextInputProps, TouchableOpacity, StyleSheet } from 'react-native'; +import { + View, + TextInput, + TouchableOpacity, + StyleSheet, + KeyboardTypeOptions, + ReturnKeyTypeOptions, + AutoCapitalize, + NativeSyntheticEvent, + TextInputSubmitEditingEventData, +} from 'react-native'; import { Eye, EyeOff, AlertCircle } from 'lucide-react-native'; import { AppText as Text } from '../common/AppText'; import { useDynamicFontSize } from '../../hooks'; /** - * Props for the MobileFormInput component + * Explicit props for the MobileFormInput component. + * + * NOTE: Do NOT reintroduce `{...rest}` or generic prop spreading here. + * If you need an additional TextInput prop, add it explicitly to this interface + * and thread it through to below. + * See docs/prop-patterns.md. */ -interface MobileFormInputProps extends TextInputProps { +interface MobileFormInputProps { /** Label text for the input field */ label: string; /** Current value of the input */ value: string; /** Callback when the input value changes */ onChangeText: (text: string) => void; + /** Placeholder text shown when input is empty */ + placeholder?: string; /** Error message to display */ error?: string; /** Hint text to display next to the label */ @@ -24,6 +41,32 @@ interface MobileFormInputProps extends TextInputProps { required?: boolean; /** Whether to use dark mode styling */ isDark?: boolean; + /** Show as a password field with visibility toggle */ + secureTextEntry?: boolean; + /** Allow multiple lines of text */ + multiline?: boolean; + /** Keyboard type for the input */ + keyboardType?: KeyboardTypeOptions; + /** Auto-capitalisation behaviour */ + autoCapitalize?: AutoCapitalize; + /** Whether auto-correction is enabled */ + autoCorrect?: boolean; + /** Auto-complete type hint for the OS */ + autoComplete?: React.ComponentProps['autoComplete']; + /** Return key label on the software keyboard */ + returnKeyType?: ReturnKeyTypeOptions; + /** Callback when the user presses the return key */ + onSubmitEditing?: ( + event: NativeSyntheticEvent + ) => void; + /** Maximum character length */ + maxLength?: number; + /** Whether the input is editable */ + editable?: boolean; + /** Test identifier for automated tests */ + testID?: string; + /** Accessibility label for screen readers */ + accessibilityLabel?: string; } export const MobileFormInput: React.FC = ({ @@ -39,7 +82,15 @@ export const MobileFormInput: React.FC = ({ secureTextEntry, multiline = false, keyboardType = 'default', - ...rest + autoCapitalize, + autoCorrect, + autoComplete, + returnKeyType, + onSubmitEditing, + maxLength, + editable, + testID, + accessibilityLabel, }) => { const [isFocused, setIsFocused] = useState(false); const [showPassword, setShowPassword] = useState(false); @@ -98,7 +149,15 @@ export const MobileFormInput: React.FC = ({ secureTextEntry={isPassword && !showPassword} multiline={multiline} keyboardType={keyboardType} - {...rest} + autoCapitalize={autoCapitalize} + autoCorrect={autoCorrect} + autoComplete={autoComplete} + returnKeyType={returnKeyType} + onSubmitEditing={onSubmitEditing} + maxLength={maxLength} + editable={editable} + testID={testID} + accessibilityLabel={accessibilityLabel} /> {isPassword && ( diff --git a/src/components/mobile/VirtualList.tsx b/src/components/mobile/VirtualList.tsx index 9e222c20..f0b90cac 100644 --- a/src/components/mobile/VirtualList.tsx +++ b/src/components/mobile/VirtualList.tsx @@ -2,13 +2,35 @@ import React, { useCallback } from 'react'; import { FlatList, FlatListProps, ViewStyle, StyleProp } from 'react-native'; import { useMemoryMonitor } from '../../hooks'; +/** + * Explicit extension props for VirtualList. + * + * NOTE: Do NOT reintroduce `{...rest}` or generic prop spreading here. + * All supported FlatList extension points must be listed explicitly. + * See docs/prop-patterns.md. + */ + export interface VirtualListProps extends Omit, 'renderItem'> { data: ReadonlyArray | null | undefined; renderItem: FlatListProps['renderItem']; keyExtractor: (item: T, index: number) => string; - itemHeight?: number; // Optional: If items have fixed height, this drastically improves layout performance + /** Optional: If items have fixed height, this drastically improves layout performance */ + itemHeight?: number; contentContainerStyle?: StyleProp; - listId?: string; // Optional ID for memory monitoring + /** Optional ID for memory monitoring */ + listId?: string; + /** Component rendered above the list items. */ + ListHeaderComponent?: FlatListProps['ListHeaderComponent']; + /** Component rendered when the list is empty. */ + ListEmptyComponent?: FlatListProps['ListEmptyComponent']; + /** Render horizontally instead of vertically. */ + horizontal?: boolean; + /** Hide the vertical scroll indicator. */ + showsVerticalScrollIndicator?: boolean; + /** Pull-to-refresh control. */ + refreshControl?: FlatListProps['refreshControl']; + /** Test identifier for automated tests. */ + testID?: string; } /** @@ -21,7 +43,13 @@ export function VirtualList({ keyExtractor, itemHeight, listId = 'VirtualList', - ...rest + contentContainerStyle, + ListHeaderComponent, + ListEmptyComponent, + horizontal, + showsVerticalScrollIndicator, + refreshControl, + testID, }: VirtualListProps) { // Monitor memory footprint based on list size useMemoryMonitor({ @@ -49,7 +77,13 @@ export function VirtualList({ maxToRenderPerBatch={10} // Reduce number of items rendered per batch windowSize={5} // Reduce the size of the render window (default 21) updateCellsBatchingPeriod={50} // Delay in ms between batch renders - {...rest} + contentContainerStyle={contentContainerStyle} + ListHeaderComponent={ListHeaderComponent} + ListEmptyComponent={ListEmptyComponent} + horizontal={horizontal} + showsVerticalScrollIndicator={showsVerticalScrollIndicator} + refreshControl={refreshControl} + testID={testID} /> ); } From e60dea54b982b12f1b3661b6a1d5b930f2566d65 Mon Sep 17 00:00:00 2001 From: T-kesh <164345961+T-kesh@users.noreply.github.com> Date: Mon, 1 Jun 2026 15:01:12 +0000 Subject: [PATCH 2/2] fix: prevent generic prop spreading in mobile drawer and offline indicator --- src/components/mobile/MobileDrawer.tsx | 18 ++++++---- src/components/mobile/OfflineIndicator.tsx | 40 +++++++++++++++++----- 2 files changed, 43 insertions(+), 15 deletions(-) diff --git a/src/components/mobile/MobileDrawer.tsx b/src/components/mobile/MobileDrawer.tsx index 06ae07b2..f4648712 100644 --- a/src/components/mobile/MobileDrawer.tsx +++ b/src/components/mobile/MobileDrawer.tsx @@ -1,17 +1,18 @@ -import React from 'react'; -import { View, Text, TouchableOpacity } from 'react-native'; import { DrawerContentComponentProps, DrawerContentScrollView, DrawerItemList, } from '@react-navigation/drawer'; -import { useSafeArea } from '../../hooks'; import { Settings, LogOut, Sun, Moon } from 'lucide-react-native'; +import React from 'react'; +import { View, Text, TouchableOpacity } from 'react-native'; + +import { useSafeArea } from '../../hooks'; /** * Props for the MobileDrawer component */ -export const MobileDrawer = (props: DrawerContentComponentProps) => { +export const MobileDrawer = ({ state, navigation, descriptors }: DrawerContentComponentProps) => { const { top, bottom } = useSafeArea(); const [isDark, setIsDark] = React.useState(false); @@ -41,8 +42,13 @@ export const MobileDrawer = (props: DrawerContentComponentProps) => { - - + + = ({ onPress = () => {}, showDetails = false, }) => { - const { isOnline, isOffline, networkStatus, refresh } = useNetworkStatus(); + const { isOnline, isOffline, refresh } = useNetworkStatus(); // Don't show when online unless explicitly requested if (isOnline && !showWhenOnline) { @@ -62,7 +63,7 @@ export const OfflineIndicator: React.FC = ({ onPress(); } } catch (error) { - logger.error('Error refreshing network status:', error); + defaultLogger.error('Error refreshing network status:', error); } }; @@ -94,14 +95,35 @@ export const OfflineIndicator: React.FC = ({ // Export simplified versions export const OfflineBanner = OfflineIndicator; -export const OnlineIndicator = (props: any) => +export const OnlineIndicator = ({ + position = 'top', + backgroundColor = '#4CAF50', + textColor, + onPress, + showDetails, +}: OfflineIndicatorProps) => React.createElement(OfflineIndicator, { - ...props, + position, + backgroundColor, + textColor, + onPress, + showDetails, showWhenOnline: true, - backgroundColor: '#4CAF50', }); -export const ConnectionQualityIndicator = (props: any) => - React.createElement(OfflineIndicator, { ...props, position: 'bottom' }); -export const OfflineFAB = (props: any) => null; // Disabled due to configuration issues +export const ConnectionQualityIndicator = ({ + position = 'bottom', + backgroundColor = '#FF5722', + textColor, + onPress, + showDetails, +}: OfflineIndicatorProps) => + React.createElement(OfflineIndicator, { + position, + backgroundColor, + textColor, + onPress, + showDetails, + }); +export const OfflineFAB = (_props: OfflineIndicatorProps) => null; // Disabled due to configuration issues export default OfflineIndicator;