|
| 1 | +# CodeRabbit PR Review Standards for Degoog |
| 2 | + |
| 3 | +Degoog is a Bun + Hono TypeScript search aggregator. As an AI code reviewer, use these standards to evaluate pull requests, ensuring the project remains maintainable without forcing rewrites or breaking public behavior. |
| 4 | + |
| 5 | +Apply these standards strictly to new code. For existing code, suggest improvements only when the module is already being touched for a feature, bug fix, or security update. |
| 6 | + |
| 7 | +## 1. Core Principles Review |
| 8 | + |
| 9 | +### Directives |
| 10 | +- **Protect Contracts:** Flag any unannounced changes to existing APIs, UI contracts, config names, environment variables, plugin/theme/engine interfaces, store layouts, and route behavior. Require a documented migration path or `@deprecated` shim. |
| 11 | +- **Scope Control:** Reject massive, style-only rewrites. Praise and encourage small, behavior-preserving changes. |
| 12 | +- **Test Enforcement:** Block refactors of routes, search orchestration, persistence, extension loading, security gates, or user settings if they lack tests covering observable behavior. |
| 13 | +- **Readability:** Push back on overly clever abstractions. Code must be readable so future contributors can trace route/search/registry behavior easily. |
| 14 | +- **Trust Boundaries:** Treat installed extensions/themes as trusted, but rigorously verify that PRs treat their inputs, paths, URLs, rendered HTML, and persisted metadata as untrusted. |
| 15 | + |
| 16 | +## 2. TypeScript Style & Module Boundaries |
| 17 | + |
| 18 | +### What to Look For |
| 19 | +- **Strict DTOs:** Verify strict types for data crossing boundaries (client/server, route/orchestration, registry/extensions). Ensure shared shapes use `src/shared` to avoid client/server drift. |
| 20 | +- **Return Types:** Flag missing explicit return types on exported functions, route helpers, registry helpers, persistence functions, and security-sensitive utilities. |
| 21 | +- **Typing Rules:** Enforce `unknown` at external boundaries (followed by validation/narrowing) over `any`. |
| 22 | +- **Naming Conventions:** |
| 23 | + - Types/Interfaces: `PascalCase`. |
| 24 | + - Constants: `UPPER_SNAKE_CASE`. |
| 25 | + - Internal Helpers: Leading underscore `_` only if file-private and matching local convention. |
| 26 | + - Verbs: `parse*`, `is*`/`assert*`, `to*`/`from*`, `load*`/`write*`. |
| 27 | +- **Function Size:** Suggest splitting functions that exceed ~60 lines or try to handle parsing, validation, persistence, rendering, and logging all at once. |
| 28 | +- **Correct Placement:** Ensure changes respect module boundaries (e.g., HTTP concerns in `routes/`, shared logic in `utils/search.ts`, UI orchestration in `client/modules/`). |
| 29 | + |
| 30 | +## 3. Hono Route Standards |
| 31 | + |
| 32 | +### Route PR Checklist |
| 33 | +- **Guard Placement:** Verify that rate limiting and auth guards (`guardApiKey`, settings guards) are placed at the *top* of the route handler, before expensive operations. |
| 34 | +- **JSON Parsing:** Flag repeated `try/catch` blocks for body parsing; suggest extracting or using existing JSON parser helpers. |
| 35 | +- **Error Envelopes:** Ensure JSON routes return consistent `{ error: string }` envelopes and appropriate status codes. Do not allow plain text errors unless the route is a binary/text proxy with an established contract. |
| 36 | +- **Separation of Concerns:** Route handlers must focus on HTTP. Suggest moving store mutations, search orchestration, and persistence into helper functions. |
| 37 | +- **Default Preservation:** Scrutinize parser refactors to ensure default values (search type, page, lang, streaming toggles) remain intact. |
| 38 | + |
| 39 | +## 4. Extension Registry Standards |
| 40 | + |
| 41 | +### Registry PR Checklist |
| 42 | +- **Determinism:** Verify that directory reads/entries are explicitly sorted to guarantee stable load order across restarts. |
| 43 | +- **ID Stability:** Enforce canonical ID structures `<folder>-<kind>` (`-engine`, `-slot`, `-command`, `-tab`, etc.). Reject renames of built-in IDs or settings IDs without a valid migration. |
| 44 | +- **Duplicate Handling:** Ensure duplicate extension IDs are handled gracefully (logged with context) and do not silently merge unrelated settings. |
| 45 | +- **Lifecycle Semantics:** `match() === null` should not log as an error. `onLoad` failures should log context without leaking secrets and safely skip the extension. |
| 46 | +- **Immutability:** Ensure callers do not mutate registry-owned arrays (`items()`). |
| 47 | + |
| 48 | +## 5. Store & Installation Standards |
| 49 | + |
| 50 | +### Installation PR Checklist |
| 51 | +- **Transparency:** Reject PRs that silently run package manager commands or hide dependency installations. |
| 52 | +- **Path Containment:** Scrutinize repository operations. Verify URL scheme validation, git error sanitization, timeouts, and containment checks (reject `..`, absolute child paths, symlink escapes). Never trust repo-provided filenames for writes. |
| 53 | +- **Atomic Writes:** Ensure persistence updates for store metadata are atomic (e.g., temp-file creation followed by rename). |
| 54 | +- **Concurrency:** Look for locks on store writes, settings writes, and install/uninstall operations to prevent race conditions. |
| 55 | +- **ID Preservation:** Ensure installed item IDs and `installedAs` names are preserved across updates unless explicitly changed by the user. |
| 56 | + |
| 57 | +## 6. Search Orchestration Standards |
| 58 | + |
| 59 | +### Search PR Checklist |
| 60 | +- **Streaming Parity:** Enforce identical orchestration paths between `/stream` and non-streaming search. Query parsing, engine selection, interceptors, scoring, and cache writes must not be duplicated or drifted. |
| 61 | +- **Cache Integrity:** Verify that cache keys include *all* inputs (query, overrides, engine config, page, time, lang, image filters). |
| 62 | +- **Interceptor Overrides:** Ensure `searchType`, `lang`, and `timeFilter` overrides from interceptors are correctly applied *before* cache key construction and engine selection. |
| 63 | +- **Timeouts/Signals:** Verify that engine fetches receive `AbortSignal` and that streaming stops when the client disconnects. |
| 64 | +- **Engine Type Model (CRITICAL):** |
| 65 | + - Reject restrictive unions for `EngineSearchType` (it must remain `string`). |
| 66 | + - Ensure type arrays (`["web", "karakeep"]`) are supported. |
| 67 | + - Verify `resolveTypes` in `engines/registry.ts` is the single source of truth for type resolution. |
| 68 | + - Verify `selectActiveEngines` uses unified paths (`getActiveWebEngines` vs `getEnginesForCustomType`). Ensure `includeCustom` is not reintroduced. |
| 69 | + |
| 70 | +## 7. Client UI Standards |
| 71 | + |
| 72 | +### Frontend PR Checklist |
| 73 | +- **Layer Separation:** Suggest splitting UI functions that mix parsing, fetching, state updates, and DOM rendering. |
| 74 | +- **Selector Stability:** Reject changes to stable DOM IDs, classes (`degoog-*`), and `data-*` attributes. These are public APIs for themes, plugins, and browser extensions. |
| 75 | +- **Event Handlers:** Praise/suggest event delegation (`data-action`) over rebinding handlers on every render. |
| 76 | +- **DOM Safety:** Strongly flag the use of `innerHTML` unless handling explicitly trusted templates or sanitized HTML. Recommend `textContent` for general data. |
| 77 | +- **Accessibility:** Ensure interactive elements are semantic (`<button>`, `<a>`), have `aria-label`s if icon-only, preserve keyboard navigation, and handle loading states visibly. |
| 78 | + |
| 79 | +## 8. Security Standards |
| 80 | + |
| 81 | +### Security PR Checklist |
| 82 | +- **SSRF Prevention:** Ensure proxied/fetched URLs strictly allow `http:` and `https:`, re-check protocols after redirects, and use signed proxy URLs for exposed assets. |
| 83 | +- **Path Verification:** Assert that all extension/store paths are resolved and checked for containment before reads/writes. |
| 84 | +- **Secret Hygiene:** Flag PRs that log settings/admin/search API tokens or nonces. Ensure secret settings are masked in UI/metadata responses. |
| 85 | +- **Header Trust:** Do not allow trust of `X-Forwarded-*` headers unless explicit proxy trust settings are enabled. |
| 86 | +- **Error Safety:** Ensure error responses do not leak local paths, tokens, repo internals, or stack traces. |
| 87 | + |
| 88 | +## 9. Persistence & Cache Standards |
| 89 | + |
| 90 | +### Persistence PR Checklist |
| 91 | +- **JSON Schema:** Ensure JSON persistence logic tolerates missing fields, preserves unknown fields, and recovers safely. |
| 92 | +- **Atomicity:** Flag direct overwrites of critical JSON files. Require atomic write patterns (write to temp file -> fsync -> rename). |
| 93 | +- **Caching:** Ensure new cache APIs use async `useCache`. Verify cache invalidation clears both local memory and Valkey state. Ensure TTLs rely on safe defaults/env vars. |
| 94 | + |
| 95 | +## 10. Logging & Observability |
| 96 | + |
| 97 | +### Logging PR Checklist |
| 98 | +- **Console Usage:** Reject raw `console.*` in server code (except for startup scripts). Enforce the central `logger` utility. |
| 99 | +- **Namespaces:** Ensure logs use feature namespaces (e.g., `search`, `store:repo`, `settings`). |
| 100 | +- **Telemetry Value:** Ensure logs contain meaningful metrics (query lengths, result counts, timings) and *never* log sensitive payloads, passwords, or tokens. |
| 101 | +- **Structured Formats:** Encourage `key=value` paired strings for easier scanning. |
| 102 | + |
| 103 | +## 11. Testing Standards |
| 104 | + |
| 105 | +### Test PR Checklist |
| 106 | +- **Coverage:** Reject bug fix PRs that lack regression tests (if testable). Demand tests for route shapes, auth guards, cache keys, and store safety. |
| 107 | +- **Isolation:** Verify tests isolate runtime data using env vars/data paths. |
| 108 | +- **Mocks:** Ensure network/git mocks are used sparingly and assert the critical commands/options. |
| 109 | +- **Determinism:** Flag flaky tests. Inputs must be sorted, time controlled, and external search dependencies mocked or removed. |
| 110 | + |
| 111 | +## 12. Duplication Control |
| 112 | + |
| 113 | +### Refactoring PR Checklist |
| 114 | +- **Rule of Two:** Do not praise generic abstractions created for a single call site. Require at least two real use cases before extracting shared helpers. |
| 115 | +- **Focus:** Prefer small, narrowly-named helpers over dumping unrelated functions into large utility files. |
| 116 | + |
| 117 | +## 13. Final Approval Gate (Rule of Thumb) |
| 118 | + |
| 119 | +Before approving a PR, verify: |
| 120 | +1. Does it preserve user-facing behavior? (Unless explicitly marked as a breaking change). |
| 121 | +2. Are compatibility risks for extensions/plugins/themes considered? |
| 122 | +3. Are secrets, paths, and HTML boundaries safely handled? |
| 123 | +4. Is the PR small enough to review confidently? (If not, suggest splitting it up). |
0 commit comments