|
| 1 | +# PR Review Guidelines for Cursor Bot (Root) |
| 2 | + |
| 3 | +**Scope & intent** |
| 4 | + |
| 5 | +- High-level review guidance for the entire 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 | + |
| 35 | +--- |
| 36 | + |
| 37 | +## 1) General Software Quality |
| 38 | + |
| 39 | +**Clarity & simplicity** |
| 40 | + |
| 41 | +- Prefer straightforward control flow, early returns, and focused functions. |
| 42 | +- Descriptive names; avoid unnecessary abbreviations. |
| 43 | +- Keep public APIs minimal and intentional. |
| 44 | + |
| 45 | +**Correctness & safety** |
| 46 | + |
| 47 | +- Add/update tests with behavioral changes and bug fixes. |
| 48 | +- Handle error paths explicitly. |
| 49 | +- Avoid global mutable state; prefer immutability and clear ownership. |
| 50 | + |
| 51 | +**DRY & cohesion** |
| 52 | + |
| 53 | +- Remove duplication where it reduces complexity; avoid over-abstraction. |
| 54 | +- Keep modules cohesive; avoid reaching across layers for convenience. |
| 55 | + |
| 56 | +**Performance (pragmatic)** |
| 57 | + |
| 58 | +- Prefer linear-time approaches; avoid unnecessary allocations/copies. |
| 59 | +- Don’t micro-optimize prematurely—call out obvious hotspots or regressions. |
| 60 | +- Use streaming/iterables over building large intermediates when feasible. |
| 61 | + |
| 62 | +--- |
| 63 | + |
| 64 | +## 2) Dart-Specific |
| 65 | + |
| 66 | +**Idioms & language features** |
| 67 | + |
| 68 | +- Avoid `!` (null assertion) on nullable targets; use guards or null-aware operators. |
| 69 | +- Follow Effective Dart style and documentation; document public symbols. |
| 70 | +- Consider modern Dart 3.x features (e.g., sealed classes) when they clarify the model. |
| 71 | + |
| 72 | +**Safety & async** |
| 73 | + |
| 74 | +- Avoid unawaited futures unless intentional and documented. |
| 75 | +- Cancel `StreamSubscription`s and dispose resources deterministically. |
| 76 | + |
| 77 | +**Concurrency & isolates** |
| 78 | + |
| 79 | +- For CPU-bound work with simple inputs/outputs, consider offloading to an **isolate**; avoid complex object graphs needing heavy serialization. |
| 80 | + |
| 81 | +**Tree-shakeability** |
| 82 | + |
| 83 | +- Avoid patterns that defeat tree shaking |
| 84 | +- Use const constructors/values where it makes sense to enable constant folding/canonicalization. |
| 85 | +- Instantiate optional features/integrations inside guarded branches (e.g., if (options.enableX) { ... }). |
| 86 | +- Prefer typed factories over dynamic/string lookups |
| 87 | + |
| 88 | +--- |
| 89 | + |
| 90 | +## 3) SDK-Specific (high-level) |
| 91 | + |
| 92 | +**Tracing & spans** |
| 93 | + |
| 94 | +- Any span started must be **closed**. |
| 95 | +- For _automated_ instrumented spans, always set: |
| 96 | + - `sentry.origin` |
| 97 | + - `sentry.op` using a standard operation where applicable (see [Sentry’s list of standard ops](https://develop.sentry.dev/sdk/telemetry/traces/span-operations/)). |
| 98 | + |
| 99 | +**Structured logs** |
| 100 | + |
| 101 | +- For _automated_ instrumented structured logs, always set `sentry.origin`. |
| 102 | + |
| 103 | +**Initialization & error paths** |
| 104 | + |
| 105 | +- Wrap dangerous or failure-prone paths (especially during `Sentry`/`SentryFlutter` init) in `try/catch`, add actionable context, and ensure fallbacks keep the app usable. |
| 106 | + |
| 107 | +--- |
| 108 | + |
| 109 | +## Quick reviewer checklist |
| 110 | + |
| 111 | +- [ ] **CRITICAL:** No secrets/PII/logging risks introduced; safe defaults preserved. |
| 112 | +- [ ] **CRITICAL:** Public API/telemetry stability maintained or properly deprecated with docs. |
| 113 | +- [ ] Spans started are always closed; automated spans/logs include `sentry.origin` (+ valid `sentry.op` for spans). |
| 114 | +- [ ] Dangerous init paths guarded; app remains usable on failure. |
| 115 | +- [ ] No `!` on nullable targets; async gaps guarded; resources disposed. |
| 116 | +- [ ] Tests/docs/CHANGELOG updated for behavior changes. |
0 commit comments