|
| 1 | +--- |
| 2 | +name: code-review-react-native |
| 3 | +description: Performs strict code reviews on React Native plugin projects covering TypeScript, the Kotlin (Android) and Swift (iOS) native bridges, and the Expo config plugin. Focuses on optimal code, readability, maintainability, deduplication, single-responsibility, straightforward control flow, React Native best practices, the rule that native layers must not invent hardcoded fallbacks for values the TypeScript layer or platform SDK already owns, alignment of identifiers and API surface across TypeScript/Kotlin/Swift (especially NativeModules name, NativeEventEmitter channel data, and EventIdentifiers), and explicit documentation of platform-specific features. Use when the user asks to "review PR", "do a code review", "review this code", "code review for PR#N", or otherwise requests review of changes in a React Native plugin — including its built output (`lib/`), Expo config plugin output (`plugin/build/`), and native source under `android/` and `ios/`. |
| 4 | +--- |
| 5 | + |
| 6 | +# Code Review (React Native) |
| 7 | + |
| 8 | +Strict, opinionated code review for React Native plugin codebases with Kotlin + Swift bridges and an optional Expo config plugin. Optimised for catching the kinds of issues that survive linters and tests but degrade a codebase over time. |
| 9 | + |
| 10 | +## Review priorities, in order |
| 11 | + |
| 12 | +1. **Correctness & contract** — bugs, breaking changes, public-API violations, SemVer; identifiers and API surface aligned across TypeScript, Kotlin, and Swift. |
| 13 | +2. **Native-layer cleanliness** — no hardcoded fallbacks for values owned by TypeScript or the platform SDK; no behaviour duplicated across language boundaries; platform-specific features explicitly documented. |
| 14 | +3. **Single responsibility** — each function, class, and file does one thing. |
| 15 | +4. **Deduplication (DRY)** — repeated parsing, serialisation, and validation patterns get extracted. |
| 16 | +5. **Readability** — naming, argument styles, generated-vs-handwritten boundaries, no surprises. |
| 17 | +6. **Maintainability** — generated files untouched, defaults documented in one place, tests for new public surface. |
| 18 | +7. **React Native / TypeScript best practices** — `const`, `@deprecated` discipline, `null` vs `undefined`, strict tsconfig honoured. |
| 19 | +8. **Polish** — JSDoc style, example brevity, changelog accuracy, PR description present. |
| 20 | + |
| 21 | +## Workflow |
| 22 | + |
| 23 | +### 1. Gather context |
| 24 | + |
| 25 | +If the user references a GitHub PR by number: |
| 26 | + |
| 27 | +```bash |
| 28 | +gh pr view <N> --json title,body,author,state,baseRefName,headRefName,additions,deletions,changedFiles,files,url |
| 29 | +gh pr diff <N> |
| 30 | +git log <base>..<head> --oneline |
| 31 | +``` |
| 32 | + |
| 33 | +If the changes are local: |
| 34 | + |
| 35 | +```bash |
| 36 | +git diff <base>..HEAD --stat |
| 37 | +git diff <base>..HEAD |
| 38 | +``` |
| 39 | + |
| 40 | +Always read the full file (not only the diff) for any non-trivial change so you see the surrounding context the diff hides — especially around native bridges, event emitter wiring, and `EventIdentifiers`. |
| 41 | + |
| 42 | +### 2. Classify every changed file |
| 43 | + |
| 44 | +Each file falls into exactly one of these buckets, and the bucket determines what's acceptable: |
| 45 | + |
| 46 | +| Bucket | Examples | Rule | |
| 47 | +|---|---|---| |
| 48 | +| Hand-written TypeScript | `src/` | Full review applies. | |
| 49 | +| Built JS output | `lib/commonjs/`, `lib/module/`, `lib/typescript/` | **Must not be hand-edited.** Regenerate via `yarn build` (react-native-builder-bob). | |
| 50 | +| Expo config plugin source | `plugin/src/` | Full review applies. | |
| 51 | +| Expo config plugin build | `plugin/build/` | **Must not be hand-edited.** Regenerate via `yarn build` or `tsc -p plugin/tsconfig.build.json`. | |
| 52 | +| Hand-written native | `android/src/main/kotlin/**`, `ios/**/*.swift` | Full review applies, with extra scrutiny on the bridge layer. | |
| 53 | +| Build / config | `package.json`, `*.gradle`, `*.podspec`, `tsconfig.json`, `babel.config.js` | Verify version bumps follow SemVer; confirm SDK/binary updates match changelog. | |
| 54 | +| Docs / release | `CHANGELOG.md`, `README.md` | Verify claims match the diff (e.g. SDK versions, breaking-change list, public-API additions). | |
| 55 | + |
| 56 | +### 3. Run the checklist below and produce the report |
| 57 | + |
| 58 | +--- |
| 59 | + |
| 60 | +## Hard rules — block the merge |
| 61 | + |
| 62 | +### 1. No hand-edits to generated / built files |
| 63 | + |
| 64 | +`lib/` (built by react-native-builder-bob) and `plugin/build/` (compiled from `plugin/src/`) must be touched only by their build tool. |
| 65 | + |
| 66 | +Red flags: |
| 67 | +- Behaviour-changing edits inside `lib/` that differ from what `yarn build` would produce. |
| 68 | +- `plugin/build/` edited directly instead of editing `plugin/src/` and re-running the build. |
| 69 | +- A `// eslint-disable` or `// @ts-ignore` directive added to a file inside `lib/`. |
| 70 | + |
| 71 | +Action: ask the author to run `yarn build` and commit the clean output. |
| 72 | + |
| 73 | +### 2. SemVer must match the change |
| 74 | + |
| 75 | +A breaking public-API change requires a major version bump. "Public" includes anything exported from `src/index.tsx` and anything that changes the wire format with the native side. |
| 76 | + |
| 77 | +Common breaks to flag: |
| 78 | +- Renamed or retyped fields on exported TypeScript types or interfaces. |
| 79 | +- Renamed string values consumers may pattern-match against. |
| 80 | +- Removed or repurposed enum variants that map to native callback codes. |
| 81 | +- Changed required/optional status on config parameters. |
| 82 | + |
| 83 | +If `CHANGELOG.md` claims SemVer adherence and the bump doesn't match the change, call it out with the affected symbols and the recommended version. |
| 84 | + |
| 85 | +### 3. No hardcoded fallbacks in the native layer |
| 86 | + |
| 87 | +The native layer (Kotlin / Swift) is a transport adapter between TypeScript and the platform SDK. It must not: |
| 88 | + |
| 89 | +- **Invent default values** for fields the TypeScript layer marks required. Such defaults are unreachable but advertise an optional contract that doesn't exist. |
| 90 | +- **Substitute defaults** for nullable/optional fields. Either the SDK has its own default (skip the call) or the default belongs in TypeScript. |
| 91 | +- **Encode the same default in multiple places.** A default as a literal in `optString("foo", "BAR")`, as an enum in `getOrDefault(Foo.BAR)`, and again as a fallback object is three sources of truth. |
| 92 | +- **Hardcode enum names as raw strings** (e.g. `"SIDELOADED_ONLY"`). Use the SDK enum's `.name` property or avoid manual parsing entirely. |
| 93 | + |
| 94 | +Recommended remediation: |
| 95 | +- For **required** TypeScript fields: drop the native default; let parsing throw and surface through the existing error path. |
| 96 | +- For **optional** TypeScript fields: skip the builder/setter call when the field is absent; let the SDK apply its own default. |
| 97 | +- Document the default once — in the TypeScript JSDoc — so the public contract is unambiguous. |
| 98 | + |
| 99 | +### 4. `NativeModules` name must be consistent across all layers |
| 100 | + |
| 101 | +The string used to register and look up the native module must agree across three places: |
| 102 | + |
| 103 | +| Layer | Location | Value | |
| 104 | +|---|---|---| |
| 105 | +| TypeScript | `src/api/nativeModules.ts` — `NativeModules.FreeraspReactNative` | `"FreeraspReactNative"` | |
| 106 | +| Kotlin | `FreeraspReactNativeModule.kt` companion — `const val NAME = "FreeraspReactNative"` | `"FreeraspReactNative"` | |
| 107 | +| iOS | `RCT_EXPORT_MODULE()` / `moduleName` override in Swift/ObjC bridge | `"FreeraspReactNative"` | |
| 108 | + |
| 109 | +Any drift in this string causes the module to be `undefined` at runtime with no compile-time error. |
| 110 | + |
| 111 | +### 5. NativeEventEmitter channel data aligned across TypeScript, Kotlin, and Swift |
| 112 | + |
| 113 | +Threats and execution state are delivered through obfuscated event channels. The channel metadata is fetched at runtime from native via two methods: |
| 114 | + |
| 115 | +| Method | Returns | Kotlin | Swift | |
| 116 | +|---|---|---|---| |
| 117 | +| `getThreatChannelData` | **3 strings**: `[channelName, threatKey, malwareKey]` | `getThreatChannelData()` promise | `getThreatChannelData(_:resolve:reject:)` | |
| 118 | +| `getRaspExecutionStateChannelData` | **2 strings**: `[channelName, key]` | `getRaspExecutionStateChannelData()` | `getRaspExecutionStateChannelData(_:resolve:reject:)` | |
| 119 | + |
| 120 | +Verify for every change touching these methods: |
| 121 | +- Array length is the same on Kotlin and Swift sides. |
| 122 | +- The semantic order of items in the array is the same on both native sides and matches what `src/channels/threat.ts` and `src/channels/raspExecutionState.ts` destructure by index. |
| 123 | +- The TypeScript type declarations in `src/types/types.ts` (`TalsecPlugin` interface) reflect any change to the array shape. |
| 124 | + |
| 125 | +### 6. `EventIdentifiers` must stay obfuscated — never replace with hardcoded strings |
| 126 | + |
| 127 | +`ios/utils/EventIdentifiers.swift` derives all channel identifiers at runtime from `RandomGenerator.generateRandomIdentifiers(length: N)`. This obfuscation is intentional and security-relevant. |
| 128 | + |
| 129 | +Hard blocks: |
| 130 | +- Replacing a `generatedNumbers[i]` reference with a hardcoded string or integer constant. |
| 131 | +- Adding a new identifier slot without expanding `length` and updating all index references consistently. |
| 132 | +- Using the same index in two different identifier roles (index collision). |
| 133 | + |
| 134 | +The Kotlin side generates its own independent random identifiers via an equivalent mechanism; they are not shared with iOS. This is by design — each process re-generates at launch. Do not attempt to synchronise iOS and Android identifier values. |
| 135 | + |
| 136 | +### 7. `NativeEventEmitter` listener registration and cleanup symmetry |
| 137 | + |
| 138 | +Every `addListener(channelName, callback)` call on `NativeEventEmitter(FreeraspReactNative)` must have a matching cleanup path. |
| 139 | + |
| 140 | +Check: |
| 141 | +- `removeListenerForEvent(channelName)` is called in Kotlin's `removeListenerForEvent` react method, and the matching Swift handler does the same. |
| 142 | +- The returned `EmitterSubscription` (or the subscription stored in `useFreeRasp`) is `.remove()`d on cleanup. |
| 143 | +- Kotlin's `addListener` increments the listener count so the native module doesn't drop events on multi-listener setups. |
| 144 | + |
| 145 | +### 8. Platform-specific features must be documented and gated |
| 146 | + |
| 147 | +Not every API works on both platforms. Some checks are Android-only (e.g. malware detection, package introspection), some iOS-only (e.g. jailbreak sub-checks). |
| 148 | + |
| 149 | +Required: |
| 150 | +- **JSDoc states the platform**: any type, field, enum variant, or callback that only works on one platform must say so — e.g. `/** Android only. No-op on iOS. */` |
| 151 | +- **Config class isolation**: platform-specific config fields belong on the platform-specific config class, not on the shared `TalsecConfig`. |
| 152 | +- **CHANGELOG calls out the platform**: bullets for added/changed/removed features must say "(Android)" or "(iOS)" when applicable. |
| 153 | + |
| 154 | +Red flags: |
| 155 | +- A `TalsecConfig`-level field read only by one of the two native handlers, with no platform marker. |
| 156 | +- A callback that fires only on one platform without a JSDoc note. |
| 157 | +- An enum variant with no native producer on one side. |
| 158 | + |
| 159 | +### 9. Expo config plugin changes are complete and tested |
| 160 | + |
| 161 | +If `plugin/src/` changes: |
| 162 | +- Verify `plugin/build/` is regenerated and committed. |
| 163 | +- Confirm the plugin modifies the correct build artifact (`AndroidManifest.xml`, `Info.plist`, Gradle properties, etc.) and does not duplicate a modification the user's `app.json` already handles. |
| 164 | +- The example app `example/` should demonstrate or at least not break the plugin path. |
| 165 | + |
| 166 | +### 10. Tests for new public API |
| 167 | + |
| 168 | +Every newly exported type, enum, or function needs at least: |
| 169 | +- Construction / instantiation test (defaults, required fields). |
| 170 | +- Round-trip serialisation test if the type crosses the JS–native bridge as JSON. |
| 171 | +- Enum-name stability test if the enum maps to native callback codes (catching drift from native renames). |
| 172 | + |
| 173 | +--- |
| 174 | + |
| 175 | +## Significant issues — should fix |
| 176 | + |
| 177 | +### Single responsibility |
| 178 | + |
| 179 | +- A function that parses, validates, defaults, and constructs in one body is doing four things. Split. |
| 180 | +- A Kotlin class with `parse*`, `build*`, and `dispatch*` methods is three classes. |
| 181 | +- A TypeScript file that mixes API methods, listener helpers, and type definitions is three modules. |
| 182 | + |
| 183 | +### Deduplication |
| 184 | + |
| 185 | +Flag verbatim or near-verbatim repetition, especially: |
| 186 | +- The same `(0 until arr.length()).map { arr.getString(it) }` pattern repeated for every JSON array field in Kotlin. Extract a helper. |
| 187 | +- The same `value ?? defaultValue` pattern repeated across multiple config fields. Use a helper or map. |
| 188 | +- Multiple `runCatching { Enum.valueOf(s) }.getOrDefault(...)` calls in Kotlin. Extract a generic `parseEnumOr(default)` helper. |
| 189 | + |
| 190 | +### Argument style for many-parameter constructors |
| 191 | + |
| 192 | +Constructors with **3+ parameters of similar type** should be called with named arguments. Positional calls are swap-bug magnets. |
| 193 | + |
| 194 | +### Old API still wired alongside the new one |
| 195 | + |
| 196 | +When deprecating an API path, ensure the new path doesn't quietly run both code paths. Either short-circuit the deprecated path or document the precedence explicitly. |
| 197 | + |
| 198 | +### `useFreeRasp` hook completeness |
| 199 | + |
| 200 | +The hook is the primary consumer entry point. If new config fields or callbacks are added, verify: |
| 201 | +- The hook's internal types accept the new fields. |
| 202 | +- Cleanup logic (in `useEffect` return) covers any new subscriptions. |
| 203 | +- The hook is re-exported from `src/index.tsx`. |
| 204 | + |
| 205 | +--- |
| 206 | + |
| 207 | +## Style & polish — call out, don't block |
| 208 | + |
| 209 | +- **Trailing newlines, formatting churn**: separate from the feature; mention but don't argue. |
| 210 | +- **Verbose examples**: example code that explicitly sets parameters to their defaults teaches nothing. Either drop the assignment or assign a non-default to demonstrate customisation. |
| 211 | +- **Inconsistent terminology**: e.g. `MalwareConfig` vs `SuspiciousAppDetectionConfig` in one PR is one term too many. Pin it before release. |
| 212 | +- **JSDoc consistency**: full sentences, end with a period, match the project's existing style. |
| 213 | +- **`@deprecated` discipline**: deprecating a field is fine; leaving the constructor accepting it with no warning is inconsistent. |
| 214 | +- **PR description**: an empty PR body for a release PR is a defect of its own. Aggregate conventional commits into a short summary. |
| 215 | +- **Changelog accuracy**: every bullet in `CHANGELOG.md` should be verifiable from `git diff <base>..HEAD`. |
| 216 | +- **Yarn vs npm**: this repo uses Yarn. Any instruction in the README or PR description to run `npm install` is wrong. |
| 217 | + |
| 218 | +--- |
| 219 | + |
| 220 | +## Output format |
| 221 | + |
| 222 | +Structure the review as: |
| 223 | + |
| 224 | +```markdown |
| 225 | +## Summary |
| 226 | + |
| 227 | +<2–3 sentences: what the PR does, overall verdict> |
| 228 | + |
| 229 | +## Blockers / Major issues |
| 230 | + |
| 231 | +### 1. <short title — one line> |
| 232 | + |
| 233 | +<context, citing file:line; show the offending snippet using a code reference> |
| 234 | + |
| 235 | +<concrete remediation> |
| 236 | + |
| 237 | +### 2. ... |
| 238 | + |
| 239 | +## Significant issues |
| 240 | + |
| 241 | +(same shape, less severe) |
| 242 | + |
| 243 | +## Minor / polish |
| 244 | + |
| 245 | +(numbered list, one to three lines each) |
| 246 | + |
| 247 | +## Recommended action |
| 248 | + |
| 249 | +<numbered, ordered list of what must change before merge> |
| 250 | +``` |
| 251 | + |
| 252 | +Rules for the report: |
| 253 | +- Cite specific files and line numbers for every issue. When showing existing code, use the `startLine:endLine:filepath` reference form so the user can click through. |
| 254 | +- Prefer one detailed example over a vague generality; if a pattern repeats, mention it once and list the other locations. |
| 255 | +- Distinguish between "this is wrong" and "I'd prefer this." Flag the first as Major, the second as Polish. |
| 256 | +- Never assert facts about a closed-source SDK without verifying. If the SDK isn't readable from the repo, phrase findings as "verify whether the SDK provides X; if so, do not duplicate it." |
| 257 | +- End with a short, ordered "Recommended action" list — the actual gating items, not a wishlist. |
0 commit comments