Imported from rcchao/dotfiles (
.config/claude/skills/paraform-review/SKILL.md). Install upstream withnpx skills add rcchao/dotfiles --skill paraform-review. Copyright stays with the author.
PR Review Patterns Guide
A running guide of review feedback patterns from senior engineers (anthnykr, batuhan-akcay-paraform, taneliang, ruibinch, owen-paraform, inni-e). Use this to self-review PRs before sending them out.
Sourced from PRs across paraform-xyz/paraform. Scan ranges: #7812 to #11882 (last updated 2026-05-18).
1. Extract shared display text into helper functions
Pattern: When the same user-facing string (or string with minor conditional variation) appears in 3+ components, extract a helper function.
Why reviewers flag this: Duplicated UI copy is easy to update in one place and forget the others. It's also a sign of copy-paste development.
What to do: Create a small helper function (not a full component unless the JSX is complex) and import it. Place it in a utils.ts file adjacent to the components that use it.
Example: suggestedRolesDescription(hasCandidate: boolean) returning conditional description text, used across CreateCuratedListModal, EditList, CopyListModal.
Source: PR #9882 (batuhan-akcay-paraform)
2. Repository methods belong in the right domain file
Pattern: A findCandidateId(candidate_user_id) method was placed in candidate_user_preference.repository.ts because the service that needed it lived in the preference domain.
Why reviewers flag this: The method queries candidate_user, not candidate_user_preference. Repository files should be organized by the table they query, not by which service calls them. Future developers looking for candidate_user queries won't find it in the preference repo.
What to do: Always place repository methods in the file matching the table being queried. If candidate_user.repository.ts exists, put candidate_user lookups there.
Source: PR #9882 (batuhan-akcay-paraform)
3. Variable names should match the domain concept, not your mental model
Pattern: A variable was named recruiterPref when it queried candidate_user_preference_source.USER_INPUT.
Why reviewers flag this: The code says USER_INPUT, the variable says recruiter. A reader has to reconcile two mental models. This is especially confusing when there are multiple preference sources.
What to do: Name variables after what the code actually does: userInputPref matches USER_INPUT, applicantPref matches APPLICANT_USER. Don't inject interpretation into variable names.
Source: PR #9882 (batuhan-akcay-paraform)
4. Replace nested ternaries with if/else for readability
Pattern: const x = a !== undefined ? a === "" ? null : a : undefined - nested ternary with three branches.
Why reviewers flag this: Nested ternaries require mental stack-tracing. Each ? and : adds cognitive load. If a reviewer has to read it twice, it's too complex for a ternary.
Rule of thumb: One level of ternary is fine. Two levels - use if/else. Three levels - definitely refactor.
Source: PR #9882 (batuhan-akcay-paraform)
5. Don't apply SQL directly to the database
Pattern: Using ALTER TABLE or inserting into _prisma_migrations directly instead of going through Prisma migration files.
Why this matters: Causes schema drift that blocks prisma migrate dev and can require a full database reset. Also breaks other branches sharing the same dev database.
What to do: Always modify prisma/schema.prisma and run npx prisma migrate dev --name <migration_name>.
6. Use tRPC over raw REST endpoints
Pattern: Using fetch("/api/...") for mutations when the codebase has tRPC.
Why reviewers flag this: Raw fetch bypasses type safety, input validation (Zod), and auth checks that tRPC provides. It's also inconsistent with the rest of the codebase.
What to do: Always check if a tRPC mutation exists or can be added. Only use raw fetch for endpoints that genuinely need to be REST (webhooks, external integrations).
7. All Prisma calls belong in repositories, not services
Pattern: Adding prisma.table.findFirst(...) directly in a service method.
Why reviewers flag this: The codebase has a strict layered architecture: Routers -> Services -> Repositories -> Prisma. Services should never import or call prisma directly.
What to do: Create a repository method and call it from the service. Use read_only_prisma for read operations where appropriate.
8. Use satisfies over as const for Prisma select/where objects
Pattern: Using as const on a Prisma select or where object instead of satisfies Prisma.SomeType.
Why reviewers flag this: as const just freezes the literal type - it doesn't verify the object matches the Prisma schema. If you misspell a field or add one that doesn't exist, TypeScript won't catch it. satisfies validates structure while still narrowing the type.
What to do: Use satisfies Prisma.SomeModelSelect or satisfies Prisma.SomeModelWhereInput instead of as const for Prisma query objects.
Source: PR #8932 (anthnykr)
9. Don't wrap queries in unnecessary transactions
Pattern: Wrapping a series of read queries and a single write in a prisma.$transaction().
Why reviewers flag this: Transactions hold database locks. If the block is just some findFirst/findMany calls and one updateMany, there's no atomicity requirement - the reads don't need to be consistent with each other in a transaction boundary. Unnecessary transactions add latency and lock contention.
What to do: Only use transactions when multiple writes need to succeed or fail together, or when reads must be consistent with subsequent writes (e.g., check-then-act patterns with race conditions).
Source: PR #8932 (anthnykr)
10. Don't create thin wrapper functions
Pattern: Creating a normalizeEmail(email) function that just calls email.toLowerCase().trim() or wraps a single existing utility.
Why reviewers flag this: A function that just delegates to another function with no added logic is noise. It adds indirection without value and makes the reader wonder what extra behavior the wrapper provides.
What to do: Call the underlying function directly. Only create a wrapper if it adds meaningful logic (validation, error handling, default args, etc.).
Source: PR #8924 (anthnykr)
11. Log all items when volume is low, not just a sample
Pattern: Logging only a sample of invalid/problematic items (e.g., invalidEmails.slice(0, 5)) in a cron or batch job that runs infrequently.
Why reviewers flag this: If the log only fires once a day or less, truncating it makes debugging harder. You end up re-running the job or adding temp logging to see the full list. Premature log optimization wastes more time than it saves.
What to do: Log the full list when the log isn't high-frequency. Only truncate for hot paths (request handlers, loops that fire thousands of times).
Source: PR #8924 (anthnykr)
12. Use enums or constants for fixed string values
Pattern: Passing string literals like "active_recruiters" or "weekly_sync" directly as function arguments when they represent a fixed set of known values.
Why reviewers flag this: String literals are easy to typo and impossible to autocomplete. If the set of valid values is known, an enum or const object provides type safety, discoverability, and refactorability.
What to do: Define an enum or as const object for fixed value sets, then use the enum member instead of the string literal.
Source: PR #8904 (anthnykr)
13. Remove unnecessary type assertions
Pattern: Adding as Prisma.applicationWhereInput or as Prisma.candidate_user_roleWhereInput to an object that TypeScript can already infer correctly.
Why reviewers flag this: Type assertions (as) bypass type checking. If the type is correct without the assertion, adding it just suppresses future type errors that could catch real bugs. It's also visual noise.
What to do: Try removing the as cast. If TypeScript doesn't complain, the assertion was unnecessary. If it does complain, fix the underlying type rather than casting.
Source: PR #7400 (anthnykr)
14. Reuse existing constants from shared modules
Pattern: Hardcoding a list of IDs or values in a component when a shared constant already exists (e.g., RECRUITER_OWNER_IDS from globalUser.ts).
Why reviewers flag this: Duplicated lists drift apart over time. When someone adds a new recruiter owner to the constant, your hardcoded list stays stale. It also signals you didn't check what utilities already exist.
What to do: Before defining any list of IDs, roles, or config values, search globalUser.ts, constants/, and utils/ for existing definitions. Use them directly.
Source: PR #7231 (anthnykr)
15. Put reusable mappings in backend, not frontend
Pattern: Mapping a list of users/IDs in a frontend component (e.g., SOL_GLOBAL_USERS.map(...) in a React component) when the backend already has the shared data.
Why reviewers flag this: Frontend mappings can't be reused by other backend services, crons, or API endpoints. Putting it in the backend (e.g., extending the constant with ...SOL_GLOBAL_USERS.map(...)) makes it available everywhere.
What to do: If a mapping is over shared domain data (users, roles, config), add it to the backend constant/utility and consume it from the frontend.
Source: PR #7231 (anthnykr)
16. Check for existing functions before writing new ones
Pattern: Writing a new repository query or utility function without checking if one already exists for the same purpose.
Why reviewers flag this: The codebase is large. There's a good chance a findCandidateById, formatSalary, or similar function already exists. Duplicate functions diverge over time and confuse future developers about which to use.
What to do: Before writing a new repo method or utility, search the codebase for similar function names and the table/model being queried. If something close exists, extend it rather than duplicating.
Source: PR #9882 (batuhan-akcay-paraform), PR #6932 (anthnykr)
17. Extract duplicate formatting/display logic
Pattern: Writing inline formatting logic (e.g., salary range formatting, date display) that looks similar to an existing utility like getRoleOTE or formatCompactSalaryRange.
Why reviewers flag this: Formatting logic with edge cases (null handling, currency symbols, compact notation) is error-prone to reimplement. If a utility already handles these cases, duplicating it means bugs get fixed in one place but not the other.
What to do: Search for existing formatting utilities in lib/utils/ and component-level utils before writing formatting logic inline. If one is close but not exactly right, extend it with a parameter rather than creating a parallel implementation.
Source: PR #6932 (anthnykr)
18. Delete unused code after refactoring
Pattern: Leaving an old component, import, or function in the codebase after it's been replaced by new code.
Why reviewers flag this: Dead code confuses future developers who wonder if it's still needed. It also inflates bundle size and creates false positives in code searches. If you replaced it, remove it in the same PR.
What to do: After any refactor, search for references to the old code. If nothing imports/calls it, delete it. Don't leave "just in case" code.
Source: PR #9897 (anthnykr - "is this gonna be removed now that it's unused?", "u can just delete this if it isn't used anymore")
19. Use tRPC mutation callbacks for side effects
Pattern: Managing toast notifications, cache invalidation, or optimistic updates with separate useEffect or manual state checks after a tRPC mutation, instead of using the mutation's built-in callbacks.
Why reviewers flag this: tRPC (via React Query) provides onSuccess, onMutate, onError, and onSettled callbacks on mutations. Using these is cleaner, avoids race conditions, and keeps side-effect logic co-located with the mutation.
What to do: Put toast notifications in onSuccess/onError, optimistic updates in onMutate, and cache invalidation in onSettled. Don't manage mutation side effects with separate state.
Source: PR #9897 (anthnykr - "you can do all this in the upsertBoost mutation using onSuccess, onMutate, onError, etc")
20. Keep routers thin, move logic to services
Pattern: Putting business logic (data transformation, conditional checks, multi-step operations) directly in a tRPC router instead of a service.
Why reviewers flag this: Routers should only handle input validation and delegation. Business logic in routers can't be reused by crons, other routers, or tests without importing the router itself.
What to do: Create a service method (e.g., FirstSubmissionBoostService.getBoostStats) and call it from the router. The router should be a thin passthrough.
Source: PR #9897 (anthnykr - "nit (non-blocking): move this to a FirstSubmissionBoostService.getBoostStats service")
21. Schema design: relations, indexes, defaults, and naming
Pattern: Creating a new Prisma model with missing foreign key relations, no indexes on FKs, unclear column names, or dangerous default values.
Why reviewers flag this: Multiple issues compound:
- Missing FK relations:
boosted_by_user_idwithout a@relationtoUsermeans no referential integrity and no Prisma relation queries. - Missing indexes: Foreign key columns without
@@indexcause slow joins at scale. - Unclear names:
subs_per_weekvsexpected_subs_per_week- the prefix makes intent clear. - Dangerous defaults:
default: trueon a new boolean column silently changes behavior for all existing rows. Ask: should existing rows really have this enabled? - Redundant columns: If
boost_reasonsis already a string array, a separateother_reasoncolumn may be unnecessary. - Indirect relations: Link to the primary entity (
Role) not a settings table, to avoid traversals likeRole -> role_settings -> first_submission_boost.
What to do: For every new model/column, check: FK relation defined? Index on FK? Name self-documenting? Default value safe for existing rows? No redundant columns?
Source: PR #9897 (anthnykr), PR #9857 (batuhan-akcay-paraform - "Do we really want this to be default true?", "Should we make this an enum?")
22. Chunk Promise.all for unbounded arrays
Pattern: Using Promise.all(items.map(async item => prisma.something.findFirst(...))) where items could be any size.
Why reviewers flag this: Prisma has a connection pool (default 5-10 connections). If items has 100 entries, you fire 100 concurrent DB queries, exhausting the pool and causing timeouts or deadlocks.
What to do: Chunk the array into batches of ~5 and process each batch with Promise.all sequentially. Use a utility like chunk() from lodash or write a simple loop.
Source: PR #9842 (anthnykr - "we generally don't wanna do a promise.all with an unbounded array size, it's gonna hit prisma's connection pool limit. chunk it, e.g. in chunks of 5")
23. Add comments for complex algorithms
Pattern: Writing a multi-step scoring algorithm, quality calculation, or raw SQL query with no comments explaining the approach.
Why reviewers flag this: Complex algorithms (especially raw SQL with multiple JOINs, CASE statements, or scoring weights) are opaque without explanation. Future maintainers (including you) won't remember why specific thresholds or weights were chosen.
What to do: Add a block comment above complex algorithms explaining: what it does, what the major components/signals are, and why key thresholds were chosen. For raw SQL, explain the query strategy.
Source: PR #9857 (batuhan-akcay-paraform - "Can we add general comments on this sql algorithm for quality scoring? How does it operate, what are major components it looks at")
24. Prefer Prisma over raw SQL
Pattern: Writing raw SQL queries in repositories when the same query could be expressed with Prisma's query builder.
Why reviewers flag this: Raw SQL bypasses Prisma's type safety, is harder to maintain, and doesn't benefit from schema changes/migrations. It also creates a mental context switch for developers reading the code.
What to do: Default to Prisma query builder. Only use raw SQL (prisma.$queryRaw) when the query genuinely can't be expressed in Prisma (complex aggregations, window functions, recursive CTEs). If you do use raw SQL, add a comment explaining why Prisma wasn't sufficient.
Source: PR #9857 (batuhan-akcay-paraform - "Do you think it would be better to convert this raw SQL query to more standard prisma logic + typescript code?")
25. Always use design system components, never legacy or custom
Pattern: Using <ButtonLegacy>, creating custom styled buttons with inline styles, or building ad-hoc input components instead of the design system equivalents.
Why reviewers flag this: The team has invested in a design system (/lib/components/ui). Using legacy or custom components:
- Creates visual inconsistency
- Misses future design system upgrades (new DS work is underway)
- Adds maintenance burden for one-off styles
- Often gets the interaction patterns wrong (focus states, a11y, variants)
What to do:
<ButtonLegacy>-><Button>from design system- Custom styled buttons ->
<Button variant="secondary">or<Button variant="destructive"> - Don't make everything
primary- only one primary action per screen - Custom inputs ->
<Input>from design system - Use
<Button block>for full-width buttons
Source: PR #9767 (taneliang - "Shouldn't use ButtonLegacy", "Shouldn't create custom buttons - strongly prefer design system components", "We have a design system Input too", "this shouldn't be primary so that we don't have multiple primary actions on the same screen")
26. Prefer early returns for invalid/edge cases
Pattern: Nesting valid-case logic inside if (condition) blocks instead of returning early for the invalid case.
Why reviewers flag this: Deep nesting makes it harder to follow the "happy path." Early returns for edge cases (null checks, invalid states, empty arrays) flatten the code and make the main logic more readable.
What to do: Flip the condition, return early, then write the main logic at the top indentation level. Example: if (!valid) return null; then proceed with the happy path un-nested.
Source: PR #9746 (ruibinch - "Early return for the invalid case is neater to read")
27. Right-size LLM output token limits
Pattern: Setting max_tokens: 512 (or higher) for an LLM call that only needs a short JSON response with a category and a one-line explanation.
Why reviewers flag this: Over-allocated token limits waste money on every API call and can cause the model to pad responses with unnecessary text. If your expected output is ~50 tokens, setting 512 is 10x over.
What to do: Estimate the expected output size and set max_tokens to ~2x that estimate. For structured JSON responses, count the fields and typical value lengths.
Source: PR #9810 (owen-paraform - "512 seems like an unnecessary number of output tokens for a json with a one word explanation and a short string")
28. Don't add redundant checks handled by called functions
Pattern: Adding a null/existence check before calling a function that already handles that check internally.
Why reviewers flag this: Redundant guards add noise and can mask the actual contract of the called function. If messageSlackChannel already checks if the channel exists before sending, adding the same check before calling it is wasted code.
What to do: Read the implementation of functions you call. If they handle edge cases internally, trust the abstraction and don't duplicate the check at the call site.
Source: PR #9739 (anthnykr - "nit: don't need this channel check, it's already done in messageSlackChannel")
29. Follow existing codebase patterns for common operations
Pattern: Updating a field like crons_last_synced with a direct Prisma update instead of using the established helper function that other crons use.
Why reviewers flag this: The codebase often has specific patterns for common operations (e.g., updateRoleLastSynced keeps updatedAt stable while updating sync timestamps). Using a different approach creates inconsistency and may break assumptions other code relies on.
What to do: Before writing a common operation (updating timestamps, sending notifications, formatting Slack messages), search for how existing code does it. Use the same pattern/helper function.
Source: PR #9773 (anthnykr - "when crons_last_synced is updated in the codebase, it usually keeps the updatedAt field stable. function - updateRoleLastSynced")
30. Use data-driven patterns over manual switch/mapping
Pattern: Writing a manual switch statement or chained if/else to map values to sort orders or categories when the same thing could be a list with indexOf.
Why reviewers flag this: Manual mappings are verbose, error-prone when new values are added, and don't self-document the ordering. A data-driven approach (define the ordered list, use indexOf) is shorter, easier to maintain, and makes the ordering explicit.
What to do: Define values in an ordered array/object, then use indexOf for sort comparisons or object lookup for mappings. Only use switch/if-else when the logic per case is truly different.
Source: PR #9746 (ruibinch - suggested defining propensity values in a list and using indexOf for ordering)
31. Consider query efficiency with early returns
Pattern: Always running an expensive multi-table query (e.g., application quality scoring with multiple JOINs) before checking a cheap condition (e.g., reviewed_count < 3).
Why reviewers flag this: If the cheap check would short-circuit the expensive query, you're wasting DB resources on every invocation. This is especially impactful in crons that run frequently.
What to do: Order your checks from cheapest to most expensive. Run simple count queries or field checks first, and only proceed to complex queries if the cheap checks pass.
Source: PR #9857 (batuhan-akcay-paraform - "Would the application quality query be more efficient if we first check the reviewed count and if less than 3 early return rather than always running the complex query?")
32. Use enums for categorical schema fields
Pattern: Storing categorical data (quality levels like "HIGH"/"LOW", status values) as plain strings in the database instead of Prisma enums.
Why reviewers flag this: Plain strings allow typos, have no autocomplete, and require magic string comparisons everywhere. Enums enforce valid values at the database level and provide TypeScript type safety.
What to do: For any field with a known, finite set of values, define a Prisma enum and use it as the column type. Migrate existing string columns to enums when practical.
Source: PR #9857 (batuhan-akcay-paraform - "Should we make this an enum? LOW, HIGH etc.?"), PR #9857 ("might want to use constants here for 'HIGH' and 3")
33. Parallelize independent async operations
Pattern: Awaiting independent async calls sequentially (const a = await getA(); const b = await getB();) when they don't depend on each other.
Why reviewers flag this: Sequential awaits add unnecessary latency. If getA() takes 200ms and getB() takes 300ms, sequential is 500ms but parallel is 300ms.
What to do: Use const [a, b] = await Promise.all([getA(), getB()]) for independent operations. But remember pattern #22 - chunk if the array is unbounded.
Source: PR #9810 (owen-paraform - "nit: no reason not to promise.all this")
34. Use camelCase for all new variables
Pattern: Declaring new variables with snake_case (const role_id = ...) instead of camelCase.
Why reviewers flag this: The team has been moving away from snake_case variables. Mixing conventions in the same file creates inconsistency and makes grep/refactoring harder. New code should follow the newer convention.
What to do: Use camelCase for all new variable declarations. Don't rename existing snake_case vars in unrelated code (that's a separate cleanup PR), but don't add new ones either.
Source: PR #8235 (taneliang - "We've been moving away from snake case vars, please use camel case for all new variables instead")
35. Remove AI-generated comments from code
Pattern: Leaving comments auto-generated by Cursor, Copilot, or other AI tools in the submitted code (e.g., // This function handles... boilerplate explanations).
Why reviewers flag this: AI-generated comments are often generic, redundant with the code itself, or outright wrong. They signal the code wasn't reviewed after generation. Reviewers shouldn't have to wonder which comments are intentional and which are AI artifacts.
What to do: Review all comments in your diff before submitting. Delete any that were auto-generated and don't add value. If a comment is useful, rewrite it in your own words to be specific.
Source: PR #8133 (taneliang - "remove Cursor comment")
36. Don't pass functions directly as array method callbacks
Pattern: Writing .map(parseInt) or .filter(isValid) instead of .map(x => parseInt(x)) or .filter(x => isValid(x)).
Why reviewers flag this: Array methods pass extra arguments (index, array) to callbacks. If the passed function accepts optional parameters, it silently receives the index as a second arg. ["1","2","3"].map(parseInt) returns [1, NaN, NaN] because parseInt gets called with (value, index). TypeScript won't catch this.
What to do: Always wrap in an arrow function: .map(x => parseInt(x)). This makes the argument passing explicit and safe.
Source: PR #8350 (taneliang - "passing reusable functions as callback arguments is unsafe - if the function is changed to accept a number as a second argument, it'll start receiving the index")
37. Keep PRs focused - separate unrelated changes
Pattern: Including eslint config changes, unrelated refactors, or test infrastructure updates in a feature PR.
Why reviewers flag this: Mixed PRs are harder to review, harder to revert, and pollute git blame. If the eslint change causes a problem, you'd have to revert the feature too. Reviewers also have to context-switch between unrelated changes.
What to do: Split unrelated changes into separate PRs. If you notice a lint issue while working on a feature, fix it in a follow-up PR. Keep each PR doing one thing.
Source: PR #8185 (naveengovind - "nit: would good to separate the es-lint changes into a separate PR", "good to move all these changes related to removing it from eslint into a separate pr")
38. Use object params when function signatures grow
Pattern: A function with 4+ positional parameters: function createRole(name, type, ownerId, settings, isLegal, companyId).
Why reviewers flag this: Positional params are easy to mix up (is ownerId the 3rd or 4th arg?), impossible to read at the call site without checking the definition, and painful to extend (adding a new param means updating every call site's argument order).
What to do: When a function has 3+ params, switch to a single object parameter: function createRole({ name, type, ownerId, settings }: CreateRoleParams). This is self-documenting at the call site and order-independent.
Source: PR #9013 (minhpg - "too many params can we use an object here?")
39. Don't use fragile string matching for conditional logic
Pattern: Checking if a paragraph ends with 'here' or 'feedback form' to decide which URL to append, instead of passing the URL as a parameter.
Why reviewers flag this: String content checks break silently when copy is edited. If someone tweaks the email template text, the URL logic breaks. This is invisible to the editor because the logic is decoupled from the template.
What to do: Pass values explicitly as parameters or config. Never branch on string content that could be edited by non-engineers (email copy, UI text, notification messages).
Source: PR #8991 (anthnykr - "this logic doesn't feel that robust by checking a paragraph ending with 'here' or 'feedback form' - this can easily break if a template is edited")
40. Use findUnique when you have a unique key
Pattern: Using prisma.table.findFirst({ where: { id: someId } }) when id is a unique/primary key.
Why reviewers flag this: findFirst implies the query might match multiple rows. findUnique signals intent (exactly one row), enables Prisma to optimize the query, and produces a clearer error if the constraint is violated.
What to do: Use findUnique whenever the where clause matches a unique constraint (primary key, @@unique fields). Only use findFirst when multiple rows could match and you want the first one.
Source: PR #8865 (owen-paraform - "this should be a findunique")
41. Check blast radius before modifying shared components
Pattern: Changing a shared component's props, behavior, or styling without checking what else uses it.
Why reviewers flag this: Shared components (modals, date pickers, tables, form inputs) can be imported by dozens of pages. A change that fixes your use case might break others. Reviewers will ask "will this affect anything else?" if you don't address it proactively.
What to do: Before modifying a shared component, search for all imports/usages. Note the impact in your PR description. If the change is risky, add a prop to opt-in to the new behavior instead of changing the default.
Source: PR #8271 (anthnykr - "just confirming will this affect anything else? this might be used in a few places")
42. Include screenshots for UI changes
Pattern: Submitting a PR with frontend/styling changes but no screenshot or screen recording in the PR description.
Why reviewers flag this: Reviewers can't verify visual changes from code alone. CSS changes especially can have unexpected effects. Without a screenshot, the reviewer has to check out your branch and navigate to the page manually.
What to do: For any PR that changes UI (layout, styling, new components, copy changes), include before/after screenshots or a short screen recording in the PR description.
Source: PR #8235 (taneliang - "The styling changes in this file look pretty gnarly, were these tested? Didn't see a screenshot/screen recording in the PR")
43. Use tRPC RouterInputs/RouterOutputs types for component props
Pattern: Manually defining a TypeScript type for props that mirror a tRPC query response, instead of deriving it from the router.
Why reviewers flag this: Manual types drift from the actual API response when the router changes. tRPC provides RouterInputs and RouterOutputs utility types that stay in sync automatically.
What to do: Import and use RouterOutputs['routerName']['procedureName'] for component props that represent API data. This ensures type safety end-to-end.
Source: PR #8221 (anthnykr - "you can simplify reminder using trpc types from RecruiterRouterOutputs")
44. Add safety nets for scripts and backfills
Pattern: Writing a backfill script or data migration loop with no error handling or max-iteration guard.
Why reviewers flag this: Scripts run against production data. A bug in the loop condition can cause infinite iteration, an unhandled error can leave data in a partial state, and without logging you won't know what happened.
What to do: Add try/catch around the main loop body, log progress at regular intervals, set a max-iteration safety cap, and consider batching with take/skip for large datasets.
Source: PR #8858 (owen-paraform - "nice to have an error catch in case this goes crazy and keeps looping somehow")
45. Match test/benchmark settings to production
Pattern: Using temperature: 1 in an LLM benchmark script when production uses temperature: 0 (or vice versa).
Why reviewers flag this: Benchmark results are meaningless if settings don't match production. Different temperature, model, or prompt versions will produce different accuracy numbers that don't reflect real-world performance.
What to do: Always mirror production LLM settings (model, temperature, max_tokens, system prompt) in benchmarks. If you intentionally diverge, document why.
Source: PR #8835 (owen-paraform - "is there a reason we're using temperature 1 here? afaik we don't do that in production")
46. Don't mix concerns in validation/eligibility checks
Pattern: Adding ParaMatch rejection logic inside the core application eligibility check function that handles standard submission validation.
Why reviewers flag this: Eligibility checks are critical path code. Mixing in unrelated domain logic (matching, scoring, AI calibration) makes the checks harder to reason about, test, and maintain. Each concern should be a separate check.
What to do: Keep each validation check focused on one concern. If you need a new check, create a new eligibility check function rather than bolting it onto an existing one.
Source: PR #8185 (naveengovind - "would be best not to modify the core logic in the application submission check by adding in extra checks", "think we should separate any logic to do with paramatch outside this core eligibility check")
47. Don't prop-drill mutate functions - use SWR mutate or useUtils at the point of use
Pattern: Passing a mutate or refetch callback down through 3+ component layers as a prop so a deeply nested child can trigger a cache invalidation.
Why reviewers flag this: Prop drilling mutate functions creates tight coupling between parent and child components, makes the prop chain fragile, and clutters every intermediate component's props with passthrough values. SWR and tRPC both provide hooks to invalidate/refetch from any component.
What to do:
- SWR: Call
useSWRwith the same key in the child, or usemutatefromswrwith the key directly - tRPC: Use
api.useUtils()(or the SWR equivalent) in the child component to get the query client and call.invalidate()or.refetch()directly - Only pass data down as props, not refetch/mutate functions
Source: PR #9922 (anthnykr - "just checking is there a cleaner way than passing this down 3 components?", "don't need to pass mutateCandidateInfo down, you can just do the SWR equivalent of api.useUtils()")
48. Flag unintended behavior changes in refactors
Pattern: Refactoring a function and accidentally changing its behavior (e.g., handleResetFilters no longer calling handleResetView) without noting it in the PR.
Why reviewers flag this: In a refactor PR, reviewers assume behavior is preserved unless stated otherwise. If a side effect is removed or a call chain is broken, the reviewer needs to know if it's intentional or a regression. Silent behavior changes are the hardest bugs to catch in review.
What to do: If your refactor changes any observable behavior, call it out explicitly in the PR description. If it's unintentional, fix it before requesting review. Diff your function's call graph before and after.
Source: PR #9922 (anthnykr - "handleResetFilters no longer calls handleResetView, is this intended?")
49. Use isLoading from useQuery, not data presence
Pattern: Checking if (!data) or if (data === undefined) to determine loading state instead of isLoading from the query result.
Why reviewers flag this: Data-presence checks conflate loading, error, and empty-result states. isLoading is the canonical loading boolean from React Query and disambiguates "still fetching" from "fetched and empty." This also avoids flicker on refetch.
What to do: Destructure isLoading (or isPending for tRPC v11) from the query and gate UI on that. Use data checks only for the rendered content.
Source: PR #11037 (anthnykr: "just check isLoading from the usequery", "check isLoading")
50. Use assertNever in switch defaults for exhaustiveness
Pattern: A switch over an enum/union with a silent default: branch (or no default) instead of default: assertNever(value).
Why reviewers flag this: When you add a new variant to the enum/union, the switch will silently fall through. assertNever makes TypeScript fail the build at every switch that hasn't been updated, turning a silent runtime bug into a compile error.
What to do: For any switch over a finite union, end with default: return assertNever(value); (or throw via the helper). Same applies to if/else if chains over discriminated unions.
Source: PR #10323 (anthnykr: "consider using an assertNever in the default for switch statements generally. the reason is because if you add a new status and forget to update this switch, the missing case will be swallowed silently")
51. Don't add fallbacks for fields that are always present
Pattern: Writing boost.amount ?? 0, application.user_id ?? "", role?.name ?? "", or (boost.amount as number) for Prisma fields that are non-nullable in the schema, or for query results where the field is guaranteed by an earlier check.
Why reviewers flag this: Defensive fallbacks for impossible-null cases hide bugs. If the value ever IS null, it silently becomes 0 or empty string or falsy and downstream logic misbehaves. They also mislead the next reader into thinking the field can be null when it can't.
What to do: Check the Prisma schema. If the column is non-nullable, drop the fallback. If a where-clause guarantees the value, drop the fallback. Use ! only as a last resort, and prefer an assert helper that throws loudly if the invariant breaks.
Source: PR #10322 (anthnykr, repeated 8+ times: "amount is always a number", "boost.amount always exists", "slots always exists"), PR #10284 (anthnykr: "user_id always exists on application records", "applicationIdToRoleRow keys are already strings, no need for a filter")
52. take without orderBy is non-deterministic
Pattern: prisma.x.findMany({ where, take: 1 }) (or any other limit) with no orderBy.
Why reviewers flag this: Without orderBy, Postgres can return rows in any order, and the row you get back can change between runs as the table grows or vacuums. If you're taking 1 to find "the latest" or "the earliest," missing the orderBy gives you a random row that often happens to be correct in testing.
What to do: Always pair take with an orderBy. Even take: 1 needs it. Pick the column that defines what "the one you want" means (usually created_at desc or a status timestamp).
Source: PR #10063 (anthnykr, flagged 7+ times in the same PR: "orderby", "if you're taking 1, don't you need to order it?", "same as other comment, dont you need an orderBy here if using take")
53. Move filtering, counting, and sorting to the backend
Pattern: Fetching a full list from a tRPC endpoint and then doing .filter() / .reduce() / .sort() in the component to compute tab counts, badge text, or derived state.
Why reviewers flag this: The client iterates the full list on every render, ships data the user never sees, and duplicates logic the backend already has. It also means the front end has to import enums and types that should be backend-only. As the dataset grows the page slows down for no reason.
What to do: Return the computed counts/groups/sort order from the backend. The frontend should consume display-ready data. Reasonable client-side work: trivial conditional styling, formatting a date. Anything that touches every row of an array belongs server-side.
Source: PR #11017 (owen-paraform: "this shit sucks and it should definitely be in the backend", "all the filtering, counting, and sorting is client-side... iterating the full list once per stage chip on every render, which is terrible"), PR #11088 (owen: "the endpoint should filter them so that the front end doesn't have to"), PR #10711 (owen: "this is the kind of thing that should ideally be calculated in the backend")
54. Don't create a new endpoint when the data fits an existing response
Pattern: Adding a new tRPC procedure to fetch a single derived field (e.g., a limit, a count, an availability boolean) that could be appended to the existing endpoint's response.
Why reviewers flag this: Each new endpoint adds a network round-trip and another auth/validation surface to maintain. If the data is always fetched alongside another query (same screen, same user, same role), the existing endpoint should return it.
What to do: Before adding a procedure, check whether the component that needs the data already calls a related endpoint. If yes, extend that endpoint's response shape. Reserve new endpoints for genuinely independent data.
Source: PR #10908 (owen-paraform: "this shouldn't be its own API call, it should just be returned on the getNextRoleClientMatch response", "as I said in the component comment, no need for this to be a new endpoint", "the front end doesn't need to know about the limit")
55. Throw tRPC errors in the router, not in the service
Pattern: Throwing new TRPCError({ code: "FORBIDDEN", ... }) from inside a service method.
Why reviewers flag this: Services should be transport-agnostic so they can be reused by crons, queues, scripts, and other routers. Throwing tRPC errors couples the service to the HTTP/tRPC layer. The service should return a status (enum, discriminated result, or throw a domain error) and the router translates it to the right tRPC code.
What to do: Have the service return data with a status field, an enum, or throw a plain domain Error. The tRPC procedure inspects the result and throws the appropriate TRPCError. Also: do auth checks in the procedure (via assertTrpcGuardianChecks or similar), not buried in service code.
Source: PR #10890 (naveengovind: "best to avoid throwing TRPC errors directly from the service... having validateRecruiterInSameAgency return enums or some form of invalid status that the router can consume"), PR #10322 (anthnykr: "we usually do auth checks in the trpc procedure instead of the service files")
56. Don't useMemo cheap computations
Pattern: Wrapping arr.length, a simple arr.filter(x => x.active), or a single arithmetic expression in useMemo.
Why reviewers flag this: useMemo has its own cost (dependency comparison, cache slot, hook bookkeeping). For cheap synchronous work, it's slower than just recomputing. It also signals the author cargo-culted memoization without measuring.
What to do: Only useMemo when the computation is genuinely expensive (loops over large arrays, deep object construction passed to memoized children) or when reference equality matters for a dependency. Otherwise just a const in the render body.
Source: PR #9946 (anthnykr: "no need for a usememo here, not an expensive calculation", "unnecessary useMemo here, this can just be a const"), PR #10172 (anthnykr: "does this need a useMemo? could just be a variable")
57. New services should be classes
Pattern: Creating a new *.service.ts file as a collection of exported functions or a plain const xService = { foo, bar } object.
Why reviewers flag this: The team is standardizing on class-based services (with private helpers as methods, shared state as fields). Class form makes the public surface explicit, enables private helpers without polluting module exports, and keeps related types/methods grouped.
What to do: Write new services as export class FooService { ... }. Move helper functions that are only used by the service into private methods. Put shared types next to the class in a colocated types.ts once there are enough of them.
Source: PR #11024 (owen-paraform: "would be nice if this was a class, as I think we should prefer that going forward", "this could be a nice private method instead of a floating helper"), PR #11173 (owen: "it's much nicer when these are classes, if you could convert it"), PR #11017 (owen: "try not to have helper functions that live outside the service object unless you can't avoid it")
58. Use the versioned JSON system for evolving JSON fields
Pattern: Storing a new shape inside an existing json Prisma column (or adding a second version of one) without going through lib/versioned/.
Why reviewers flag this: Once a JSON column has more than one historical shape, readers need a way to know which version a row is and migrate on read. The codebase has a versioning system in lib/versioned/ precisely for this. Adding new keys ad-hoc creates undocumented mixed-shape rows that are painful to reason about.
What to do: For any JSON column expected to evolve, define versioned types under lib/versioned/<Field>/ with a version discriminator and reader functions that handle each version. For columns unlikely to change, still type the shape via Zod inline.
Source: PR #11529 (anthnykr: "now that there's more than 1 version of the json field it'd be good to use the versioning system in the codebase, lib/versioned/..."), PR #11226 (anthnykr: "if you expect this json field to change quite often then it may be worth using the json versioning system, lib/versioned")
59. Don't use updated_at as a proxy for a state-change date
Pattern: Treating updated_at on a row as "the date this row reached status X" (e.g., querying application.updated_at to find hire date, paused date, etc.).
Why reviewers flag this: updated_at bumps on ANY field change: notes, tags, ATS sync, internal flags. A row that was hired last month can re-enter a "hired this week" window just because someone touched an unrelated field. The number is silently wrong.
What to do: Use the audit/history table that records the state transition explicitly (e.g., application_audit with status = HIRED, take the earliest created_at). The existing first_reach CTE pattern in pacing queries is the canonical approach.
Source: PR #11270 (HARI-PRMD: "windowHires uses updated_at on application as a proxy for hire date, but updated_at bumps on any field change... Use application_audit to find the first row with status=HIRED"), PR #10014 (naveengovind: "probably shouldn't use updated_at for this as the expiration cut off since there is multiple places that could update this")
60. Don't import tRPC client error types into frontend components
Pattern: import { TRPCClientError } from "@trpc/client" in a component to check if (err instanceof TRPCClientError && err.data?.code === "...").
Why reviewers flag this: Pulling tRPC internals into UI code couples component code to the transport layer and adds bundle weight for a check that's usually doable by inspecting the error message. If you really need a stable signal, expose it as a string constant both ends share.
What to do: Filter on the error message string, or define a shared constant for the error message in a constants file that both the procedure and the component import. Don't reach into tRPC's class hierarchy from the frontend.
Source: PR #10908 (owen-paraform: "don't import trpc client error into the front end, it's good enough to filter on the message. if you really want to prevent drift you can establish the message as a constant")
61. Set cron timezone in the cron config, not in the handler
Pattern: Reading the current time in UTC inside a cron handler and calling .tz("America/Los_Angeles") to convert before doing PST-based date math.
Why reviewers flag this: Cron schedulers (GCP Cloud Scheduler, Vercel cron) accept a timezone in the cron definition. Setting it there means dayjs() inside the handler already starts in the right zone, the schedule firing time is unambiguous, and there's one source of truth for "what timezone does this cron run in."
What to do: Configure the cron entry to run in the target timezone and remove the manual conversion inside the handler. now.year(), now.startOf('day'), etc. then work correctly without extra .tz() calls.
Source: PR #11090 (anthnykr: "small cleanup, the cron timezone can just be set to PST already, don't need to manually convert timezones", "similar to other comment above, pst can be set in the cron timezone")
62. Run schema migrations in a separate PR before the code change
Pattern: Shipping a Prisma migration that adds/renames columns in the same PR as the code that reads/writes those columns.
Why reviewers flag this: During deployment, the migration and the new code don't land atomically. There's a window where either the old code is running against the new schema or the new code is running against the old schema, which can cause production errors for a few minutes. Splitting the migration first lets prod settle on the new schema before behavior changes.
What to do: Open a PR with just the migration (additive changes only: new columns nullable, new tables, new indexes). Merge and deploy. Then open the follow-up PR with the code that uses the new columns. For destructive changes, reverse: ship code that no longer references the column, then a separate PR to drop it.
Source: PR #11435 (inni-e: "I think doing migrations in tandem with code changes using the new columns causes downtime in prod for a few minutes. May be best to do your migrations first in a separate PR and then do this code change")
63. Search for built-in utilities before reimplementing string helpers
Pattern: Writing inline s.charAt(0).toUpperCase() + s.slice(1), count === 1 ? "match" : "matches", name.split(" ")[0], or s.toLowerCase().trim().
Why reviewers flag this: The codebase ships capitalize, capitalizeAllWords, pluralize, parseFirstName, validHttpUrl, validLinkedinUrl, and getSlackErrorMessage (among others). Reimplementing them inline guarantees inconsistent edge cases (Unicode capitalization, hyphenated names, irregular plurals) and means bug fixes in one place don't propagate.
What to do: Before writing a string transform, grep lib/utils/ for a likely name (pluralize, capitalize, parseFirstName, formatX). Use the existing helper.
Source: PR #10011 (anthnykr: "some useful util functions for the future: capitalize capitalizeAllWords pluralize"), PR #10284 (anthnykr: "for future reference you can use our parseFirstName function", "hari made a getSlackErrorMessage function to use"), PR #11364 (anthnykr: "for the future you can use pluralize(...)"), PR #11525 (anthnykr: "i think there's functions for this already, search something like validHttpUrl, validLinkedinUrl")
64. Use Bedrock Anthropic, not the Vercel AI SDK wrapper
Pattern: Importing anthropic from @ai-sdk/anthropic (Vercel) or calling raw streamText/generateObject from ai directly inside a service.
Why reviewers flag this: The codebase has a para-ai service that wraps the Bedrock Anthropic client with retry, logging, and model routing. Bypassing it means missing those guarantees and creating two parallel ways to call Claude.
What to do: Always go through the para-ai service methods. If you genuinely need a new capability, add it inside para-ai rather than reaching for the raw Vercel function.
Source: PR #11646 (owen-paraform: "please use bedrock anthropic, not the vercel one", "please do not use these raw vercel functions; please use the methods defined in para-ai service")
65. Use findUnique on composite unique indexes too
Pattern: Using findFirst({ where: { application_id, payment_type } }) when payment has a @@unique([application_id, payment_type]) composite index.
Why reviewers flag this: Composite unique constraints support findUnique via the generated { application_id_payment_type: { ... } } key. Using findFirst discards that guarantee and skips Prisma's optimization.
What to do: Check the schema for @@unique([...]) on the model. If your where matches it, use findUnique with the composite key shape.
Source: PR #11090 (anthnykr: "payment has a unique composite index so you can findunique on application_id + payment_type")
66. Use Statsig guardian.isOn for feature flags, not legacy flag systems
Pattern: Checking a feature flag via the old custom system, or reading flag config out of a settings object, instead of guardian.isOn("flagName", { companyId }).
Why reviewers flag this: The team is migrating to Statsig. Guardian's isOn is the standard interface, supports targeting attributes (company, user), and is the path forward. New flag checks should use it.
What to do: For any new feature flag check, use guardian.isOn("flagName", { companyId }) (or relevant attributes). Don't add new usages of the legacy flag system.
Source: PR #11090 (anthnykr: "should be able to check if a flag is enabled by just doing guardian.isOn("flag", { companyId } )"), PR #10793 (charan-karthik-paraform: "we'll be moving to statsig for feature flags so this may need to be updated")
67. Don't read localStorage synchronously in render; use useClientState
Pattern: Calling localStorage.getItem(...) directly in a component body or initial state to bootstrap UI state.
Why reviewers flag this: Synchronous localStorage reads break SSR (no window on the server) and cause hydration mismatches. The codebase ships useClientState which initializes the value lazily on the client and avoids both problems.
What to do: Use const [val, setVal] = useClientState(defaultValue, () => readFromStorage(...)) instead of touching localStorage directly in render or useState initializers.
Source: PR #11088 (owen-paraform: "reading localstorage synchronously", "should use useClientState, e.g const [dismissed, setDismissed] = useClientState(false, () => isDismissed(roleId, userId));")
68. Use dayjs for all date math, not native Date
Pattern: new Date(), Date.now(), manual ms arithmetic, or someDate.toISOString() in business logic.
Why reviewers flag this: The codebase standardizes on dayjs for timezone handling, formatting, and arithmetic. Mixing native Date and dayjs leads to subtle bugs (timezone defaults, off-by-one days) and inconsistency.
What to do: Use dayjs() / dayjs.utc() / dayjs.tz() for all date construction and manipulation. Convert to ISO strings via .toISOString() on a dayjs object only at the boundaries.
Source: PR #11088 (owen-paraform: "Native Date, use dayjs")
69. Frontend-only utilities belong in a client utils file
Pattern: Adding a function used only by the frontend (e.g., isValidImageSrc) to lib/utils/utils.ts (which is shared with the server).
Why reviewers flag this: Shared utils bundles get pulled into both server and client builds. Functions only used in the frontend should live in a client-side utils file so the server bundle stays lean and the import graph reflects actual usage.
What to do: If a util is only consumed by React components, put it in a client-side utils file (the codebase has several). Reserve lib/utils/utils.ts for genuinely cross-cutting helpers.
Source: PR #11088 (owen-paraform: "this thing is only used in the front end, should be written in a client side utils file (there are many)")
70. Colocate a types.ts file when a service or component dir grows
Pattern: Defining 5+ types inline across foo.service.ts, foo.service.test.ts, and the component file that consumes them.
Why reviewers flag this: Once types are shared across enough files, scattering them creates circular imports and forces every consumer to drill into the service file. A colocated types.ts keeps them discoverable.
What to do: When a service/feature folder has enough types to clutter the main file, move them to <feature>/types.ts and import from there. Standard layout: submission_request.service.ts, submission_request.service.test.ts, types.ts.
Source: PR #11017 (owen-paraform: "there are enough of these that there should probably be a types file"), PR #10959 (owen: "might be worth adding an exception flagger/types.ts file for all of this stuff now that there's so much")
71. Avoid hex literals in className strings; centralize design tokens
Pattern: Tailwind arbitrary-value classes with raw hex codes: border-[#fcd5b9], bg-[#fff1e6], text-[#b14f1c].
Why reviewers flag this: Hex literals duplicated across components drift from the design system, can't be themed, and aren't auditable. Reviewers will repeat the same comment in follow-up rounds until they're centralized.
What to do: Define color tokens in the Tailwind config or a shared constants file and reference them by name. If a one-off color is truly needed, extract it to a named constant near the component.
Source: PR #11017 (owen-paraform: "STATUS_STYLES and all the badge inline class strings still use raw hex literals... These should be defined somewhere not inline")
72. Pass an object to functions that need extensible mode/variant args
Pattern: A function like runMatchingCheck(candidate, role, true) where the boolean toggles between "display" and "generation" modes.
Why reviewers flag this: Positional booleans are unreadable at the call site and don't scale when a third mode appears. An object with a discriminated mode field ({ mode: "hmDisplay" } vs { mode: "generation" }) is self-documenting and extensible.
What to do: When a function has a mode/variant parameter, use an object parameter with a named field (often a union literal). Add new modes by extending the union, not by adding more positional booleans.
Source: PR #11088 (owen-paraform: "this should definitely take an object as arguments, and it should be possible to specify which kind of check you want to run, e.g. hmDisplay, generation, etc.")
73. Trim whitespace on all user-input string fields in Zod schemas
Pattern: z.string().min(1) on a tRPC input without .trim(), allowing strings like " " to pass validation.
Why reviewers flag this: Untrimmed input lets leading/trailing whitespace into the database, breaks equality checks, and creates "invisible" duplicates. Recruiter signup and similar flows have hit this repeatedly.
What to do: Default to z.string().trim().min(1) for any string the user types. Only skip .trim() when leading/trailing whitespace is semantically meaningful.
Source: PR #11539 (michaelchang-paraform: "internal_user_id: z.string().trim().min(1).optional()... I've seen a few places like in the initial recruiter signup flow where we aren't taking whitespace into consideration")
74. Extract magic numbers shared across UI, toast, and logger into a
Truncated - read the full file at https://github.com/rcchao/dotfiles/blob/5ede5c1d9ad368eba97e82d479da6d6914826024/.config/claude/skills/paraform-review/SKILL.md.