WikifitaGitHub live67e8de5
outro · sprite-mobile/sprite-mobile-code-review

Sprite Mobile Code Review — Bugs, Dead Code & Quality Assessment

Systematic code review of the Knocklace Collection app: 3 confirmed bugs, dead code inventory, code quality assessment, and improvement recommendations.

Baixar raw

Sprite Mobile Code Review — Bugs, Dead Code & Quality Assessment

Source: Reverse-engineering of /Users/alefita/workdir/sprite-mobile/ Version: 4.24.0 (JS) / 4.24.1 (native module) Severity scale: CRITICAL > HIGH > MEDIUM > LOW > INFO


1. Confirmed Bugs

Bug 1: setNotificationPermission Updates Wrong Slice State

Severity: HIGH File: src/store/permissions/reducer.ts (lines 37-43) Impact: Notification permission state is never correctly tracked; bluetooth permission state is silently overwritten when notification permission is checked.

The Bug:

// CURRENT (BUGGY)
setNotificationPermission: (state, action) => {
  state.bluetooth.granted = action.payload.granted;      // WRONG
  state.bluetooth.canAskAgain = action.payload.canAskAgain;  // WRONG
}

The Fix:

// CORRECT
setNotificationPermission: (state, action) => {
  state.notification.granted = action.payload.granted;
  state.notification.canAskAgain = action.payload.canAskAgain;
}

Root Cause: Copy-paste error from the adjacent setBluetoothPermission reducer, which correctly writes to state.bluetooth. The notification permission thunk dispatches this action, so every time the app checks notification permissions, it silently corrupts the bluetooth permission state.

Downstream Effects:

  • state.notification is always { granted: false, canAskAgain: true } (initial state) regardless of actual permission
  • state.bluetooth gets overwritten with notification permission data — if the user grants notifications but denies bluetooth, the app thinks bluetooth is granted
  • Any UI component reading permissions.notification.granted will display incorrect state
  • The requestPermissions orchestrator thunk may skip requesting bluetooth if it reads the corrupted notification state as bluetooth-granted

Workaround: The app still functions because BLE scanning proceeds regardless of the permission slice state on iOS (permissions are auto-granted). On Android, this could cause the bluetooth permission prompt to never appear if notification permission is granted first.


Bug 2: Hardcoded Portuguese in Unpair Alert

Severity: LOW File: src/components/ConnectionStateManagerView/ConnectionStateManagerView.tsx (lines 48-49) Impact: Non-Portuguese users see Portuguese button labels in the unpair confirmation dialog.

The Bug:

Alert.alert(
  t('unpair-device'),           // correctly internationalized
  t('unpair-device-message'),   // correctly internationalized
  [
    { text: 'Cancelar', style: 'cancel' },     // HARDCODED PORTUGUESE
    { text: 'Continuar', onPress: () => ... },  // HARDCODED PORTUGUESE
  ]
);

The Fix:

Alert.alert(
  t('unpair-device'),
  t('unpair-device-message'),
  [
    { text: t('cancel', 'Cancel'), style: 'cancel' },
    { text: t('continue', 'Continue'), onPress: () => ... },
  ]
);

Context: The i18n system has 13 translation keys but is missing cancel and continue. The developer likely wrote this quickly in Portuguese and forgot to add the keys. The rest of the component correctly uses t().

Severity rationale: LOW because the primary market is Brazilian (Sprite brand campaign), and the en_BR locale maps to Portuguese content anyway. But it violates the i18n contract established elsewhere in the codebase.


Bug 3: BLE Toggle onValueChange Is a No-Op

Severity: MEDIUM File: src/components/ConnectionStateManagerView/ConnectionStateManagerView.tsx Impact: The bluetooth toggle switch in the device connection view does nothing when tapped.

The Bug:

<Switch
  value={false}                    // always off
  onValueChange={() => {}}         // empty handler
  // ... styling props
/>

Analysis: The switch:

  1. Always renders as "off" (value={false}) regardless of actual bluetooth state
  2. Has an empty onValueChange handler — tapping it triggers no action
  3. Does not dispatch setBluetoothEnabled() or any BLE state action

Possible Intentions:

  • Abandoned feature: The toggle was meant to enable/disable BLE scanning but was never completed
  • UI placeholder: Left in during development for visual layout purposes
  • Permission gate: Meant to trigger bluetooth enable/disable flow but the native bridge was never wired

Context: The showBluetothState prop (note: typo "Bluetoth" in the prop name) controls whether this switch renders at all. It is currently passed as true in RadarScreen, meaning users see and can interact with a completely non-functional toggle.

Recommendation: Either wire the toggle to BleManager.enable() / disable scanning, or remove it entirely. A non-functional toggle in a production app erodes user trust.


2. Dead Code Inventory

2.1 Unused npm Dependencies

PackageStatusEvidence
@react-native-masked-view/masked-viewUnusedNot imported in any .ts, .tsx, .js, or .jsx file
its-fineUnusedReact internals hook library; zero imports found in source

@react-native-masked-view/masked-view is typically used for gradient text effects or partial image reveals. It may have been used in a previous iteration of the UI and was left in package.json after the feature was removed.

its-fine is a niche library by Poimandres (pmndrs) that provides useFiber() and useContextBridge() hooks for accessing React fiber internals. Its presence suggests someone planned to do advanced React tree manipulation (possibly for the Skia integration) but never followed through.

2.2 Unused Babel Module Aliases

The babel.config.js defines 7 module aliases via babel-plugin-module-resolver:

AliasTargetStatus
@screens./src/screens/indexActive
@components./src/components/indexActive
@assets./assets/indexActive
@store./src/store/indexActive
@theme./src/theme/indexDEAD — directory does not exist
@api./src/api/indexDEAD — directory does not exist
@hooks./src/hooks/indexDEAD — directory does not exist

Three of seven aliases point to nonexistent directories. If any import uses @theme, @api, or @hooks, it will fail at module resolution time with a confusing error. These are likely remnants from a project scaffold or boilerplate.

2.3 Dormant Kalman Filter (iOS)

Files:

  • modules/app-module/ios/KalmanFilter.swift
  • modules/app-module/ios/Matrix.swift
  • modules/app-module/ios/KalmanFilterType.swift
  • modules/app-module/ios/DoubleExtension.swift

A complete matrix-based Kalman filter implementation using Apple's Accelerate framework (vDSP, cblas_dgemm, LAPACK dgetrf_/dgetri_). The implementation includes:

  • State prediction and update matrices
  • Covariance tracking
  • Matrix inversion via LU decomposition
  • Full Swift type system integration

However, the AppModule.filter() method in AppModule.swift uses a simple sliding window mean:

func filter(rssi: Int, variation: Int) -> Int {
    positionsArray.append(rssi)
    if positionsArray.count > variation {
        positionsArray.removeFirst()
    }
    let uniqueValues = Set(positionsArray)
    return uniqueValues.reduce(0, +) / uniqueValues.count
}

The Kalman filter is never called from the active code path. It appears to be a more sophisticated implementation that was prototyped but replaced with the simpler algorithm — possibly because the sliding window mean performed adequately for the 3-7 meter range, or because the Kalman filter's matrix inversion overhead was not justified given the sampling rate.

Impact: ~200 lines of dead Swift code, including a dependency on the Accelerate framework that is compiled but never executed.

2.4 Video Component Legacy

The react-native.config.js marks a Video component as unstable_reactLegacyComponentNames for both platforms:

module.exports = {
  unstable_reactLegacyComponentNames: ['Video'],
};

This suggests the app previously used react-native-video for animated backgrounds before migrating to Skia's useAnimatedImageValue() for Android and static PNGs for iOS. The legacy component registration is a leftover.


3. Code Quality Assessment

3.1 Architecture Quality: B+

Strengths:

  • Clean separation between Redux state, BLE service layer (ApplicationService), and UI components
  • Event-driven architecture via eventemitter3 decouples BLE events from React rendering
  • Singleton ApplicationService pattern avoids prop drilling for BLE state
  • Platform-specific file resolution (.android.tsx suffix) for Skia vs. ImageBackground

Weaknesses:

  • ApplicationService is 798 lines — a god object handling BLE lifecycle, distance calculation, alert logic, remote config, and Redux coordination
  • No dependency injection — ApplicationService is a singleton with direct BleManager and store references
  • The store subscription pattern (mirroring Redux state into instance variables) bypasses React's rendering model

3.2 State Management: B

Strengths:

  • Redux Toolkit with proper slice separation (application, bluetoothAdapter, permissions)
  • redux-persist with sensible blacklist/whitelist configuration
  • Serializable check middleware correctly ignoring redux-persist actions

Weaknesses:

  • The store subscription anti-pattern: ApplicationService subscribes to the entire store and copies values into instance variables. This creates two sources of truth (Redux store and service instance variables)
  • isAlertAlreadyShown, isAlertActive, and notificationAlreadyShownForRange are all boolean/array flags managing a state machine that would be clearer as an explicit state machine pattern
  • The permissions slice bug (Bug 1) indicates the slice was not integration-tested

3.3 i18n: C+

Strengths:

  • Proper i18next setup with native locale detection
  • Fallback language chain (en_BR -> pt -> en)
  • Module-level initialization (runs before React renders)

Weaknesses:

  • Only 13 translation keys — the notification system has its own inline locale detection (isbr check) outside the i18n system
  • Missing keys for "Cancel" and "Continue" (Bug 2)
  • The en_BR locale convention is unusual and may confuse future developers
  • No pluralization support, no interpolation, no namespaces — barely using i18next's capabilities

3.4 BLE Implementation: B+

Strengths:

  • Correct use of allowDuplicates: true for continuous RSSI
  • Per-device calibration via remote config (rssiAt1Meter, rssiPow)
  • Platform-aware device identification (name on iOS, MAC on Android)
  • Background scanning on both platforms with proper service declarations
  • Remote config with bundled fallback

Weaknesses:

  • The locally forked react-native-ble-plx will not receive upstream updates — maintenance burden
  • No connection retry logic (the app is RSSI-only, so this is less critical)
  • The BLE toggle no-op (Bug 3) suggests incomplete feature integration

3.5 Build & CI/CD: A-

Strengths:

  • 5 GitHub Actions workflows covering the full distribution matrix
  • Proper signing for both platforms (keystore for Android, provisioning for iOS)
  • Firebase App Distribution for alpha, Play Store beta track, TestFlight for iOS beta
  • Node 22, Yarn with corepack, dependency caching

Weaknesses:

  • releaseLocal variant (release with debug keystore) could be a security risk if accidentally used for distribution
  • No automated testing workflow visible (no Jest/Detox step in CI)

3.6 Component Quality: B

Strengths:

  • Consistent use of React.memo for expensive renders (BackgroundVideo, RadarBaseAndEffect)
  • react-native-reanimated for smooth 60fps animations (radar sweep, button springs)
  • Proper forwardRef usage where needed (UrlButton)

Weaknesses:

  • Mixed animation libraries: react-native-reanimated (spring animations) alongside react-native's built-in Animated (toggle thumb). Should standardize on one
  • No error boundaries
  • No accessibility props (accessibilityLabel, accessibilityRole) on interactive elements

4. Improvement Recommendations

4.1 Immediate Fixes (P0)

#ActionEffort
1Fix setNotificationPermission reducer to update state.notification5 min
2Add cancel/continue i18n keys and use in unpair alert15 min
3Either implement BLE toggle functionality or remove the Switch component30 min

4.2 Cleanup (P1)

#ActionEffort
4Remove @react-native-masked-view/masked-view from dependencies5 min
5Remove its-fine from dependencies5 min
6Remove dead babel aliases (@theme, @api, @hooks)10 min
7Decide on Kalman filter: activate it or delete the dead Swift files30 min
8Clean up unstable_reactLegacyComponentNames in react-native config5 min

4.3 Architecture Improvements (P2)

#ActionEffort
9Refactor ApplicationService into smaller, focused services (BLE, Distance, Alert, Config)2-4 hours
10Replace store subscription pattern with React hooks (useSelector)1-2 hours
11Consolidate animation libraries to react-native-reanimated only1-2 hours
12Move notification text into the i18n system30 min
13Add error boundaries around screen components1 hour
14Add accessibility props to all interactive elements1-2 hours

5. Security Observations

5.1 API Key Exposure

The .env file contains API_URL and API_KEY for the Google Cloud Run remote config endpoint. These are baked into the app bundle via react-native-dotenv. While the API likely serves only read-only configuration, the key is extractable from the APK/IPA.

5.2 Remote Config Trust

The app trusts the remote config payload completely — device names, calibration values, and feature flags are all server-controlled. A compromised Cloud Run endpoint could:

  • Redirect all proximity alerts to a phishing URL via defaultVideo
  • Disable alerts entirely via enableDot: false
  • Poison distance calibration to make devices appear closer/farther than reality

5.3 Emergency URL Injection

The emergencyUrl field accepts any string and is opened via Linking.openURL(). While the user sets this value, a malicious remote config could set defaultEmergencyUrl to a javascript: or custom scheme URL. React Native's Linking.openURL does perform some scheme validation, but this is implementation-dependent.


Cross-References