|
| 1 | +# PR Review Guidelines for Cursor Bot |
| 2 | + |
| 3 | +**Scope & intent** |
| 4 | + |
| 5 | +- High-level review guidance for the entire Sentry React Native SDK monorepo. |
| 6 | +- Optimize for **signal over noise**: only comment when there's material correctness, security/privacy, performance, or API-quality impact. |
| 7 | +- If you find anything to flag, mention that you flagged this in the review because it was mentioned in this rules file. |
| 8 | +- Do not flag the issues below if they appear only in tests. |
| 9 | + |
| 10 | +**Reviewer style** |
| 11 | + |
| 12 | +- Be concise. Quote exact lines/spans and propose a minimal fix (tiny diff/code block). |
| 13 | +- If something is subjective, ask a brief question rather than asserting. |
| 14 | +- Prefer principles over nitpicks; avoid noisy style-only comments that don't impact behavior. |
| 15 | + |
| 16 | +--- |
| 17 | + |
| 18 | +## 0) Critical Issues to Flag |
| 19 | + |
| 20 | +> Use a clear prefix like **CRITICAL:** in the review comment title. |
| 21 | +
|
| 22 | +### A. Security & Privacy |
| 23 | + |
| 24 | +- **Secrets / credentials exposure**: Keys, tokens, DSNs, endpoints, or auth data in code, logs, tests, configs, or example apps. |
| 25 | +- **PII handling**: New code that logs or sends user-identifiable data without clear intent and controls. These must be gated behind the `sendDefaultPii` flag. |
| 26 | +- **Unsafe logging**: Request/response bodies, full URLs with query secrets, file paths or device identifiers logged by default. |
| 27 | +- **File/attachments**: Large or sensitive payloads attached by default; lack of size limits or backoff. |
| 28 | +- **Debug code shipped**: Diagnostics, sampling overrides, verbose logging, or feature flags accidentally enabled in production defaults. |
| 29 | + |
| 30 | +### B. Public API & Stability |
| 31 | + |
| 32 | +- **Breaking changes**: Signature/behavior changes, renamed/removed symbols, altered nullability/defaults, or event/telemetry shape changes **without** deprecation/migration notes. |
| 33 | +- **Behavioral compatibility**: Silent changes to defaults, sampling, or feature toggles that affect existing apps. |
| 34 | +- **Native bridge compatibility**: Changes to native module method signatures (iOS `RCT_EXPORT_METHOD` / Android `@ReactMethod`) must be backward-compatible or versioned, as they affect all consumers including Expo and bare React Native apps. |
| 35 | + |
| 36 | +### C. Dependency Updates |
| 37 | + |
| 38 | +- **Native SDK updates**: For PRs prefixed with `chore(deps):` updating native SDKs (e.g., `chore(deps): bump sentry-cocoa to v9.x.x`, `chore(deps): bump sentry-android to v8.x.x`): |
| 39 | + - Read the PR description which should contain the changelog. |
| 40 | + - Review mentioned changes for potential compatibility issues in the current codebase. |
| 41 | + - Flag breaking API changes, deprecated features being removed, new requirements, or behavioral changes that could affect existing integrations. |
| 42 | + - Check if version bumps require corresponding changes in the native bridge code (Objective-C/Swift on iOS, Java/Kotlin on Android). |
| 43 | +- **JavaScript dependency updates**: For PRs updating JS/TS dependencies, check for breaking API changes that affect the SDK's public surface or internal usage. |
| 44 | + |
| 45 | +--- |
| 46 | + |
| 47 | +## 1) General Software Quality |
| 48 | + |
| 49 | +**Clarity & simplicity** |
| 50 | + |
| 51 | +- Prefer straightforward control flow, early returns, and focused functions. |
| 52 | +- Descriptive names; avoid unnecessary abbreviations. |
| 53 | +- Keep public APIs minimal and intentional. |
| 54 | + |
| 55 | +**Correctness & safety** |
| 56 | + |
| 57 | +- Add/update tests with behavioral changes and bug fixes. |
| 58 | +- Handle error paths explicitly; never let a Sentry instrumentation error crash the host app. |
| 59 | +- Avoid global mutable state; prefer immutability and clear ownership. |
| 60 | + |
| 61 | +**DRY & cohesion** |
| 62 | + |
| 63 | +- Remove duplication where it reduces complexity; avoid over-abstraction. |
| 64 | +- Keep modules cohesive; avoid reaching across layers for convenience. |
| 65 | + |
| 66 | +**Performance (pragmatic)** |
| 67 | + |
| 68 | +- Prefer linear-time approaches; avoid unnecessary allocations/copies. |
| 69 | +- Don't micro-optimize prematurely—call out obvious hotspots or regressions. |
| 70 | +- Be mindful of main-thread work in React Native; offload heavy work to native threads where possible. |
| 71 | + |
| 72 | +--- |
| 73 | + |
| 74 | +## 2) TypeScript/JavaScript-Specific |
| 75 | + |
| 76 | +**Idioms & language features** |
| 77 | + |
| 78 | +- Use optional chaining (`?.`) and nullish coalescing (`??`) over manual null checks. |
| 79 | +- Avoid `any`; prefer `unknown` with explicit narrowing. |
| 80 | +- Use `async/await` over raw Promises for readability. |
| 81 | +- Follow the existing single-quote string style and 120-character line limit. |
| 82 | + |
| 83 | +**Safety & async** |
| 84 | + |
| 85 | +- Wrap `NativeModules` calls in try/catch; native bridges can throw. |
| 86 | +- Ensure Promises are handled; avoid unhandled rejections. |
| 87 | +- Check that `NativeModules.RNSentry` exists before calling methods (module may not be linked). |
| 88 | + |
| 89 | +**Tree-shakeability** |
| 90 | + |
| 91 | +- Avoid patterns that defeat tree shaking (e.g., side-effectful top-level code). |
| 92 | +- Use named exports; avoid re-exporting entire namespaces unnecessarily. |
| 93 | +- Instantiate optional integrations lazily (inside guarded branches). |
| 94 | + |
| 95 | +--- |
| 96 | + |
| 97 | +## 3) React Native Bridge (Native Modules) |
| 98 | + |
| 99 | +**iOS (Objective-C / Swift)** |
| 100 | + |
| 101 | +- New `RCT_EXPORT_METHOD` / `RCT_EXPORT_BLOCKING_SYNCHRONOUS_METHOD` must have a corresponding JS implementation. |
| 102 | +- Prefer `RCTPromiseResolveBlock`/`RCTPromiseRejectBlock` over synchronous returns for non-trivial work. |
| 103 | +- Wrap native calls in `@try/@catch` and reject the promise with a meaningful error code. |
| 104 | +- Nullability annotations (`nullable`/`nonnull`) must be consistent with JS-side expectations. |
| 105 | +- New Objective-C classes must use the `RNSentry` prefix. |
| 106 | + |
| 107 | +**Android (Java / Kotlin)** |
| 108 | + |
| 109 | +- New `@ReactMethod` entries must have a corresponding JS implementation. |
| 110 | +- Use `Promise` for async operations; call `promise.resolve()` or `promise.reject()` exactly once. |
| 111 | +- Avoid blocking the JS thread; offload heavy work to background threads. |
| 112 | +- Add `@Nullable` / `@NonNull` annotations consistently. |
| 113 | +- New classes must live under `io.sentry.react.*`. |
| 114 | + |
| 115 | +**TurboModules / New Architecture** |
| 116 | + |
| 117 | +- Changes to the native module spec (`NativeSentry.ts` or equivalent) must be reflected in both the legacy and new architecture implementations. |
| 118 | +- Verify that new methods are added to the codegen spec so they work with TurboModules. |
| 119 | + |
| 120 | +--- |
| 121 | + |
| 122 | +## 4) SDK-Specific (high-level) |
| 123 | + |
| 124 | +**Tracing & spans** |
| 125 | + |
| 126 | +- Any span started must be **closed** (including on error paths). |
| 127 | +- For _automated_ instrumented spans, always set: |
| 128 | + - `sentry.origin` |
| 129 | + - `sentry.op` using a standard operation where applicable (see [Sentry's list of standard ops](https://develop.sentry.dev/sdk/telemetry/traces/span-operations/)). |
| 130 | + |
| 131 | +**Structured logs** |
| 132 | + |
| 133 | +- For _automated_ instrumented structured logs, always set `sentry.origin`. |
| 134 | + |
| 135 | +**Initialization & error paths** |
| 136 | + |
| 137 | +- Wrap dangerous or failure-prone paths (especially during `Sentry.init`) in `try/catch`, add actionable context, and ensure fallbacks keep the app usable. |
| 138 | +- Never let SDK initialization failure crash the host application. |
| 139 | + |
| 140 | +**Replay & sensitive data** |
| 141 | + |
| 142 | +- Any new UI instrumentation must respect the masking/unmasking API. |
| 143 | +- Default to masking sensitive views; opt-in to unmasking. |
| 144 | + |
| 145 | +--- |
| 146 | + |
| 147 | +## Quick reviewer checklist |
| 148 | + |
| 149 | +- [ ] **CRITICAL:** No secrets/PII/logging risks introduced; safe defaults preserved. |
| 150 | +- [ ] **CRITICAL:** Public API/telemetry stability maintained or properly deprecated with docs. |
| 151 | +- [ ] **CRITICAL:** For dependency updates (`chore(deps):`), changelog reviewed for breaking changes or compatibility issues. |
| 152 | +- [ ] Native bridge methods (iOS & Android) are consistent with JS-side calls and handle errors safely. |
| 153 | +- [ ] TurboModule/New Architecture spec updated if native module interface changed. |
| 154 | +- [ ] Spans started are always closed; automated spans/logs include `sentry.origin` (+ valid `sentry.op` for spans). |
| 155 | +- [ ] Dangerous init paths guarded; app remains usable on failure. |
| 156 | +- [ ] `NativeModules.RNSentry` existence checked before use; async bridge calls wrapped in try/catch. |
| 157 | +- [ ] Tests/docs/CHANGELOG updated for behavior changes. |
0 commit comments