How to Code Review as a Senior Engineer: C#, SQL and SPA Applications

How to Code Review as a Senior Engineer: C#, SQL and SPA Applications

When I review code as a senior engineer, I am not trying to prove that I am clever or catch someone out. A good code review is a quality gate, a mentoring opportunity and a protection mechanism for the product.

The question I keep in mind is simple:

If this code goes to production, will it be correct, maintainable, secure, observable and reasonably performant?

Formatting, naming and compilation matter, but they are only the surface. A senior review also examines business correctness, edge cases, failure behaviour, data access, security, production diagnostics, test coverage and architectural fit. This guide applies that mindset to three common areas: C# and .NET, SQL and data access, and SPA frontends.

Review the Change Before Reviewing the Syntax

Begin with the pull request's purpose. What user or business problem is it solving? What acceptance criteria define success? Which parts of the system can it affect? A beautifully written implementation of the wrong behaviour is still wrong.

Read the ticket, API contract and important tests before concentrating on individual lines. Trace one realistic workflow, including failure paths. Then classify comments by impact: correctness or security issues should block approval; maintainability concerns should explain their future cost; small suggestions should not masquerade as production risks.

A useful review comment describes the consequence and a possible direction:

This query materialises every application before filtering. On a large lender it can
consume significant memory and transfer unnecessary rows. Could we keep it as an
IQueryable and apply the predicate, projection and pagination in SQL Server?

That is more actionable than “bad query” and more respectful than rewriting the developer's work without explanation.

Part 1 — Reviewing C# and ASP.NET Core

Start With the HTTP Contract

Consider a controller that creates a loan application:

[HttpPost]
public async Task<IActionResult> CreateLoanApplication(
    CreateLoanApplicationRequest request)
{
    var result = await _loanService.CreateAsync(request);
    return Ok(result);
}

It compiles, but the review has only begun. Where is validation? How are missing products, unauthorised users and invalid amounts represented? Is cancellation propagated? Does creating a resource warrant 201 Created? Can an exception leak an internal message?

A clearer boundary might be:

[HttpPost]
public async Task<ActionResult<LoanApplicationDto>> CreateLoanApplication(
    CreateLoanApplicationRequest request,
    CancellationToken cancellationToken)
{
    var command = new CreateLoanApplicationCommand(
        request.LoanProductId,
        request.ApplicantName,
        request.ApplicantEmail,
        request.RequestedAmount,
        request.AnnualIncome);

    var result = await _mediator.Send(command, cancellationToken);

    return CreatedAtAction(
        nameof(GetLoanApplicationById),
        new { id = result.Id },
        result);
}

The endpoint translates HTTP into an application request, passes cancellation and returns a meaningful response. Validation and known failure outcomes still need a consistent design, usually supported by central exception handling and Problem Details.

Keep Business Rules Out of Controllers

This action owns too many responsibilities:

[HttpPost("{id:int}/approve")]
public async Task<IActionResult> Approve(int id)
{
    var application = await _db.LoanApplications.FindAsync(id);

    if (application!.Status == "Rejected")
        return BadRequest("Cannot approve rejected application");

    application.Status = "Approved";
    await _db.SaveChangesAsync();
    await _emailService.SendAsync(application.Email, "Approved", "...");

    return Ok();
}

The controller knows the persistence mechanism, status transition and notification workflow. A command handler or application service gives the use case a clear home, while a domain method can protect the transition:

public void Approve()
{
    if (Status == LoanApplicationStatus.Rejected)
        throw new InvalidOperationException(
            "A rejected application cannot be approved.");

    Status = LoanApplicationStatus.Approved;
}

The senior question is not whether every project must use CQRS. It is whether important behaviour has one visible, testable owner instead of being scattered across controllers, services and frontend code.

Inspect Async, Cancellation and Resource Ownership

Blocking asynchronous work is a common production smell:

var result = _service.GetDataAsync().Result;
_service.ProcessAsync().Wait();

In ASP.NET Core this consumes thread-pool threads while the request waits and can contribute to starvation under load. Prefer async all the way down:

var result = await _service.GetDataAsync(cancellationToken);

Check that the token reaches EF Core, HttpClient and other cancellable I/O. Also ask whether apparently independent calls really can run concurrently. Multiple EF Core operations must not run concurrently on the same DbContext, which is not thread-safe.

Review service lifetimes and disposal. A scoped DbContext must not be captured by a singleton. Streams, database readers and other disposable resources need clear ownership. Fire-and-forget work should not be launched from a request without a durable queue or supervised background process.

Protect Nullability and Domain Invariants

Nullable reference types should communicate the design rather than produce warnings that the team suppresses. If Name is required, make that invariant explicit at construction or validate it at the boundary. Avoid using the null-forgiving operator merely to silence uncertainty.

Likewise, strings are weak substitutes for domain concepts:

application.Status = "Approved";

An enum prevents spelling errors; an Approve() method can additionally enforce valid transitions. Value objects can be useful for concepts such as money, email addresses or reference numbers when they genuinely centralise rules—without turning every primitive into ceremony.

Review Data Access From the C# Side

This code may load an entire table and filter it in memory:

var applications = await _repository.GetAllAsync();
var pending = applications
    .Where(x => x.Status == LoanApplicationStatus.Pending)
    .ToList();

Keep filtering, projection, ordering and pagination in the database. Watch for premature ToListAsync(), hidden IEnumerable conversion, lazy-loading N+1 queries and broad Include() graphs.

Repeated queries inside a loop deserve particular attention:

foreach (var item in items)
{
    var customer = await _customers.GetByIdAsync(
        item.CustomerId,
        cancellationToken);
}

This can create one query per item. Consider a single set-based query, batching or a projection that produces the response directly.

Logging, Errors and Tests

Logs should be structured and useful:

_logger.LogInformation(
    "Processing loan application {ApplicationId} for customer {CustomerId}",
    applicationId,
    customerId);

Do not log access tokens, credentials, bank details or unnecessary personal data. Confirm that failures can be correlated across the request and dependencies without exposing internals to the client.

Avoid catching every exception and returning 400 Bad Request. Validation failures, conflicts, missing resources, unavailable dependencies and unexpected faults are different outcomes. Central handling with safe Problem Details responses produces a more reliable API contract.

Tests should protect behaviour rather than implementation detail. Look for important business rules, authorisation boundaries, validators, handler outcomes, idempotency, concurrency cases and failure paths. A test suite that only proves constructors and property assignments adds little confidence.

Part 2 — Reviewing SQL Server and EF Core

Review for Production Data Volume

A query that behaves well with 500 development rows may collapse at 50 million. Ask how many rows exist now, how quickly the table grows, which parameters are common and what latency the user journey requires.

This query returns an unbounded, unnecessarily wide result:

SELECT *
FROM dbo.LoanApplications
WHERE Status = 'Pending'
ORDER BY CreatedAt DESC;

Project only what the consumer needs and paginate with a deterministic order:

SELECT
    Id,
    ReferenceNumber,
    ApplicantName,
    RequestedAmount,
    Status,
    CreatedAt
FROM dbo.LoanApplications
WHERE Status = @Status
ORDER BY CreatedAt DESC, Id DESC
OFFSET (@PageNumber - 1) * @PageSize ROWS
FETCH NEXT @PageSize ROWS ONLY;

Including Id makes the ordering stable when several rows share the same timestamp. For deep or frequently changing result sets, consider keyset pagination rather than assuming large offsets will remain cheap.

Treat Index Suggestions as Design Decisions

An index for the example might begin with the equality predicate and ordering columns:

CREATE INDEX IX_LoanApplications_Status_CreatedAt_Id
ON dbo.LoanApplications (Status, CreatedAt DESC, Id DESC)
INCLUDE (ReferenceNumber, ApplicantName, RequestedAmount);

This is a candidate, not an automatic answer. Review selectivity, existing indexes, read frequency, write cost, storage and maintenance. Verify the actual execution plan and compare logical reads before and after. SQL Server's missing-index recommendation is evidence to investigate, not a production instruction.

Check SARGability and Data Types

Applying a function to an indexed column can prevent an efficient seek:

WHERE YEAR(CreatedAt) = 2026

Prefer a range:

WHERE CreatedAt >= '20260101'
  AND CreatedAt <  '20270101'

Check parameter types too. Comparing an integer column with an nvarchar parameter can introduce an implicit conversion, poor estimates or an avoidable scan. The C# property, EF mapping, procedure parameter and database column should agree.

Understand Joins and Result Shape

Before approving a join, identify its cardinality. Joining applications to payments returns one row per payment, not necessarily one row per application:

SELECT a.Id, a.ReferenceNumber, p.PaymentDate, p.Amount
FROM dbo.LoanApplications AS a
JOIN dbo.Payments AS p ON p.ApplicationId = a.Id
WHERE a.Status = @ApprovedStatus;

That may be correct, or it may duplicate applications unexpectedly in the API. Review whether the consumer needs detailed rows, an aggregate, an existence check or separate bounded queries. Confirm join keys and foreign-key indexes support the access path.

Optional-filter procedures also deserve plan analysis:

WHERE (@Status IS NULL OR Status = @Status)
  AND (@LoanType IS NULL OR LoanType = @LoanType)

This convenient shape may produce weak plans for varied combinations and skewed data. Depending on evidence, parameterised dynamic SQL, recompilation or separate query shapes may be appropriate. Concatenating untrusted values is never an acceptable solution.

Look Beyond the Query Text

For important or changed queries, inspect the actual plan and measure STATISTICS IO and STATISTICS TIME with representative parameters. Examine logical reads, CPU, elapsed time, estimates versus actual rows, scans, seeks, lookups, sorts, spills and memory grants.

High elapsed time with low CPU may indicate waiting rather than expensive execution. Check blocking chains, deadlocks, long transactions and relevant wait types. Query Store can reveal regressions, plan changes and parameter-sensitive behaviour that a single local execution misses.

Keep Transactions Short and Migrations Deployable

Do not hold a database transaction open while calling an email provider, waiting for user input or performing unrelated slow work. Long transactions retain locks, increase blocking and make failure recovery harder.

Migration review is operational review. Ask:

• Can the old and new application versions run during a rolling deployment?
• Will the change scan or lock a large table?
• Does existing data need a controlled backfill?
• Should a non-null constraint be introduced in stages?
• Could removing or renaming a column break reports or integrations?
• What is the roll-forward or rollback approach?

Prefer Purposeful EF Core Projections

Broad includes can create cartesian growth and oversized payloads:

var applications = await db.LoanApplications
    .Include(x => x.Customer)
    .Include(x => x.Payments)
    .Include(x => x.Documents)
    .ToListAsync(cancellationToken);

For a read screen, project the required shape:

var applications = await db.LoanApplications
    .Where(x => x.Status == LoanApplicationStatus.Pending)
    .OrderByDescending(x => x.CreatedAt)
    .Select(x => new LoanApplicationListItemDto
    {
        Id = x.Id,
        ReferenceNumber = x.ReferenceNumber,
        ApplicantName = x.ApplicantName,
        RequestedAmount = x.RequestedAmount,
        Status = x.Status,
        PaymentCount = x.Payments.Count
    })
    .AsNoTracking()
    .Take(100)
    .ToListAsync(cancellationToken);

Then inspect the generated SQL rather than assuming that readable LINQ guarantees an efficient database operation.

Part 3 — Reviewing SPA Applications

Review the Complete User State Machine

A frontend is not complete merely because it displays successful data. Review loading, error, empty, success, unauthorised, offline and retry behaviour where relevant.

function LoanProductsPage() {
  const { products, isLoading, isError, error } = useLoanProducts();

  if (isLoading) return <LoadingState message="Loading loan products…" />;
  if (isError) return <ErrorState title="Could not load loan products" message={error.message} />;
  if (products.length === 0) return <EmptyState message="No loan products are available." />;

  return <LoanProductList products={products} />;
}

Ask whether requests are cancelled or ignored after unmount, errors are converted into user-safe messages, retry behaviour is controlled and server state is cached consistently. In a larger React application, a query library can centralise this lifecycle. In Angular, observable composition, the async pipe, signals or a disciplined NgRx design can do the same.

Put State in the Right Place

Use a simple rule:

Keep state as local as possible, but shared as necessary.

A modal's open flag or input value usually belongs locally. Remote server data benefits from a consistent fetching and invalidation strategy. Authentication state may be application-wide. A multi-step business workflow may justify a store. Putting every value in Redux or NgRx increases coupling; duplicating shared state across components creates disagreement.

For Angular, check for duplicate actions and effects, selector memoisation, nested subscriptions and manual subscriptions that outlive the component. For React, check hook rules, dependency arrays, stale closures and effects that accidentally refetch on every render.

Review Lists, Forms and Rendering Cost

Stable identity matters:

applications.map(application => (
  <ApplicationRow key={application.id} application={application} />
))

Array indexes are unsafe keys when rows can be inserted, removed or reordered. Large lists need server-side pagination or virtualisation; memoising a component does not make rendering 20,000 DOM rows sensible.

Forms need client validation for usability and server validation for authority. Review submit state, duplicate submission, field and form-level errors, keyboard behaviour and recovery after a failed request:

<button type="submit" disabled={isSubmitting}>
  {isSubmitting ? "Submitting…" : "Submit application"}
</button>

Do not reach for memoisation automatically. First identify a real rendering cost with profiling. Also watch for mutations such as calling .sort() directly on state; derive a copied array when sorting locally.

Separate Frontend Convenience From Security

A route guard improves navigation and prevents confusing UI access, but users control code running in their browser. The API must enforce authentication, authorisation and tenant boundaries for every protected operation.

Frontend guards are user experience.
Backend authorisation is security.

Review output encoding, unsafe HTML insertion, token storage, dependency risk and accidental exposure of secrets or personal data. No secret can be protected by placing it in a frontend bundle.

Include Accessibility in “Done”

Buttons should be buttons, inputs need associated labels, focus should move predictably and the experience should be operable with a keyboard. Do not communicate status through colour alone. Error messages should identify the problem and, where possible, how to correct it.

<button type="button" (click)="save()">Save</button>

Using semantic elements gives keyboard and assistive-technology behaviour that a clickable div does not provide automatically.

Part 4 — A complete production review lab

The earlier sections list the questions. Now let us review one change from first principles.

The product request is: “An organisation administrator can lock several selected user accounts after a project closes.” The pull request includes a React page, an ASP.NET Core endpoint, EF Core code and a SQL migration. It looks small in the ticket. It is not small in production.

Junior reviewer: Where do I start? The diff has 1,400 lines.

Senior reviewer: Start with the outcome and the risk boundary. Do not read top-to-bottom looking for style mistakes. Decide what must be true when this change is live.

• Only an authorised administrator may lock eligible accounts in their organisation.
• A protected account, such as the last organisation owner or the requester, cannot be locked.
• A retry after a lost response cannot create repeated business effects.
• The UI explains selected, excluded, queued, successful and failed targets.
• A large operation does not hold an HTTP request or database transaction open while waiting for the identity provider.
• Support can find and reconcile one operation without seeing unnecessary personal data.

Review the change map before code

Draw the sequence:

Admin selects users
  → browser submits command with request ID
  → API authenticates/authorises and creates durable operation
  → worker resolves each eligible target and calls identity provider
  → database stores target outcomes + audit
  → UI polls/receives operation status
  → support traces operation/request ID

This exposes where each guarantee belongs. The React button cannot protect tenant access. The API cannot assume one provider call is instant. A SQL unique constraint cannot make a browser confirmation understandable. Review becomes an end-to-end activity.

Review lab 1: the HTTP command contract

The initial endpoint:

[HttpPost("users/lock")]
public async Task<IActionResult> LockUsers(LockUsersRequest request)
{
    await _lockService.LockAsync(request.UserIds);
    return Ok();
}

This hides most decisions. Review questions:

• Which organisation/resource does the command apply to?
• Who is the caller, and how is membership checked?
• How many IDs are allowed?
• What is the idempotency key?
• Is the action synchronous or accepted for background processing?
• How are protected, missing, already locked and failed users represented?
• What response lets a client reconcile uncertain delivery?
• What does Ok() promise?

public sealed record CreateBulkLockRequest(
    Guid RequestId,
    IReadOnlyList<Guid> UserIds,
    string Reason);

public sealed record BulkLockAcceptedResponse(
    Guid OperationId,
    string Status,
    int RequestedCount,
    int EligibleCount,
    int ExcludedCount,
    string StatusUrl);

app.MapPost("/api/organisations/{organisationId:guid}/user-lock-operations",
    async Task<IResult> (
        Guid organisationId,
        CreateBulkLockRequest request,
        ICreateBulkLockOperation handler,
        CancellationToken cancellationToken) =>
    {
        var result = await handler.HandleAsync(
            organisationId, request, cancellationToken);

        return result switch
        {
            CreateOperationResult.Accepted x => Results.Accepted(
                x.StatusUrl, x.Response),
            CreateOperationResult.Duplicate x => Results.Ok(x.Response),
            CreateOperationResult.Validation x => Results.ValidationProblem(x.Errors),
            CreateOperationResult.Forbidden => Results.Forbid(),
            _ => Results.Problem()
        };
    });

202 Accepted is honest when processing continues. It does not mean success. A response with durable operation identity lets the UI and support check status later.

Review comment example

[Blocking] This command has no organisation path/scope or idempotency identity, so a client timeout can repeat a lock and the service has no durable operation to reconcile. Can we model it as a scoped operation command with request ID and return an accepted operation resource? Please include success, duplicate and partial-processing contract examples.

Review lab 2: resource authorisation and target policy

The first service implementation:

public async Task LockAsync(IEnumerable<Guid> userIds)
{
    var users = await db.Users
        .Where(x => userIds.Contains(x.Id))
        .ToListAsync();

    foreach (var user in users)
    {
        user.IsLocked = true;
    }

    await db.SaveChangesAsync();
}

It trusts IDs from the browser and loads users across all organisations. An authenticated caller could submit another organisation's IDs. It silently skips missing IDs, locks protected accounts and does no audit or concurrency handling.

Review the authorisation path:

public async Task<CreateOperationResult> HandleAsync(
    Guid organisationId,
    CreateBulkLockRequest request,
    CancellationToken cancellationToken)
{
    var actor = currentActor.RequireAuthenticated();

    await authorization.EnsureCanManageUsersAsync(
        actor, organisationId, cancellationToken);

    var candidates = await users.GetByIdsInOrganisationAsync(
        organisationId, request.UserIds, cancellationToken);

    var policy = lockPolicy.Evaluate(actor, candidates);
    // Create only eligible targets; retain exclusions for response/audit.
}

EnsureCanManageUsersAsync is coarse organisation capability. lockPolicy applies resource rules: self-lock prevention, last-owner protection, service account constraints and current state. The final worker must re-evaluate where policy can change between submission and execution.

Do not return detailed information about out-of-organisation IDs if that enables enumeration. The contract may report a generic ineligible count while audit/support controls retain authorised detail.

Junior reviewer: The frontend hides the Lock action for protected users. Is that enough?

Senior reviewer: It improves UX. A caller can bypass any client code. Review server policy and direct API tests.

Review lab 3: idempotency and durable operations

An unreliable pattern:

if (await db.BulkLockOperations.AnyAsync(x => x.RequestId == request.RequestId))
    return existing;

db.BulkLockOperations.Add(operation);
await db.SaveChangesAsync(cancellationToken);

Two concurrent requests can both see no record. Database uniqueness is the authoritative race barrier:

builder.Entity<BulkLockOperation>()
    .HasIndex(x => new { x.OrganisationId, x.RequestId })
    .IsUnique();

Store a canonical fingerprint of command meaning. If the same request ID arrives with different user IDs/reason, reject it rather than returning the first operation silently. Persist request/operation/audit/outbox records in one local transaction.

await using var transaction = await db.Database.BeginTransactionAsync(cancellationToken);

db.BulkLockOperations.Add(operation);
db.BulkLockTargets.AddRange(targets);
db.AuditEvents.Add(BulkLockAudit.Create(actor, operation));
db.OutboxMessages.Add(OutboxMessage.For(new BulkLockRequested(operation.Id)));

await db.SaveChangesAsync(cancellationToken);
await transaction.CommitAsync(cancellationToken);

The worker consumes the outbox message. It can receive the message more than once, so target processing needs idempotent/conditional state transitions. Do not send email or call the provider inside the transaction.

What the reviewer asks for

• a concurrent duplicate-request test against the real relational provider;
• a different-payload/same-request-ID test;
• a crash between commit and publish test;
• a replayed-message test;
• retention and safe lookup policy for idempotency records.

Review lab 4: async workers and external dependencies

The initial implementation processes all users in the API request:

foreach (var user in users)
{
    await identityProvider.LockAsync(user.ExternalId, cancellationToken);
    user.IsLocked = true;
    await db.SaveChangesAsync(cancellationToken);
}

For a few users, this may appear fine. For thousands, it produces request timeouts, holds resources, complicates retries and makes partial outcome invisible.

Review a bounded worker:

public sealed class BulkLockWorker(
    IServiceScopeFactory scopeFactory,
    ILogger<BulkLockWorker> logger) : BackgroundService
{
    protected override async Task ExecuteAsync(CancellationToken stoppingToken)
    {
        await foreach (var message in queue.ReadAllAsync(stoppingToken))
        {
            await using var scope = scopeFactory.CreateAsyncScope();
            var processor = scope.ServiceProvider
                .GetRequiredService<IBulkLockProcessor>();

            await processor.ProcessAsync(message.OperationId, stoppingToken);
        }
    }
}

Within processing, set operation status atomically, take a bounded page of pending targets, apply per-target timeout/rate-limit policy, record result and continue. Use short-lived DbContext per unit of work. Do not share one context across parallel tasks.

Classify provider outcomes: success, already locked, temporary throttle/unavailable, permanent invalid target, forbidden configuration and unknown. Retry only safe transient categories with bounded backoff/jitter. A provider timeout may mean lock succeeded but the response was lost; use provider idempotency/correlation/status lookup where possible before retrying.

The operation should become Completed, CompletedWithFailures, Failed or Cancelled according to product policy. “Failed” must not hide a mixture of successful locks and unresolved targets.

Review lab 5: SQL schema, queries and migration

The migration adds:

CREATE TABLE BulkLockOperations
(
    Id uniqueidentifier NOT NULL PRIMARY KEY,
    RequestId uniqueidentifier NOT NULL,
    Status nvarchar(max) NOT NULL,
    CreatedAt datetime NOT NULL
);

Review findings:

• no organisation scope or unique idempotency constraint;
• nvarchar(max) for bounded status impairs clarity/storage/indexing;
• datetime has older precision/range behaviour; choose project timestamp convention, commonly datetime2 or datetimeoffset;
• no target table/audit/outbox association;
• no FK/indexes/retention design;
• no deployment/rollback plan.

CREATE TABLE Identity.BulkLockOperation
(
    OperationId uniqueidentifier NOT NULL,
    OrganisationId uniqueidentifier NOT NULL,
    RequestId uniqueidentifier NOT NULL,
    RequestFingerprint binary(32) NOT NULL,
    Status varchar(32) NOT NULL,
    RequestedCount int NOT NULL,
    EligibleCount int NOT NULL,
    CreatedAtUtc datetime2(3) NOT NULL,
    CompletedAtUtc datetime2(3) NULL,

    CONSTRAINT PK_BulkLockOperation PRIMARY KEY (OperationId),
    CONSTRAINT UQ_BulkLockOperation_Organisation_Request
        UNIQUE (OrganisationId, RequestId),
    CONSTRAINT CK_BulkLockOperation_Status
        CHECK (Status IN ('Queued', 'Running', 'Completed',
                          'CompletedWithFailures', 'Failed', 'Cancelled'))
);

CREATE TABLE Identity.BulkLockTarget
(
    OperationId uniqueidentifier NOT NULL,
    UserId uniqueidentifier NOT NULL,
    Status varchar(32) NOT NULL,
    FailureCode varchar(64) NULL,
    UpdatedAtUtc datetime2(3) NOT NULL,

    CONSTRAINT PK_BulkLockTarget PRIMARY KEY (OperationId, UserId),
    CONSTRAINT FK_BulkLockTarget_Operation
        FOREIGN KEY (OperationId)
        REFERENCES Identity.BulkLockOperation (OperationId)
);

CREATE INDEX IX_BulkLockTarget_Pending
ON Identity.BulkLockTarget (Status, OperationId)
WHERE Status IN ('Queued', 'RetryPending');

Exact type/schema naming follows the project. The reviewer asks whether the index matches the worker query and whether status/target history belongs in this table or an audit event table.

Migration review is a release review. Additive tables/indexes are usually easier than destructive changes, but build/rehearse on representative data. The deploy order must let old and new API/worker versions coexist.

Review lab 6: query correctness at scale

A status endpoint contains:

var operation = await db.BulkLockOperations
    .Include(x => x.Targets)
    .SingleAsync(x => x.Id == operationId, cancellationToken);

return new BulkLockStatusDto
{
    Status = operation.Status,
    Targets = operation.Targets.ToList()
};

For 10,000 targets, every poll transfers/materialises all results. Review the user question: the summary needs counts and recent failures; a paginated target endpoint can provide detail on demand.

var summary = await db.BulkLockOperations
    .Where(x => x.OperationId == operationId && x.OrganisationId == actor.OrganisationId)
    .Select(x => new BulkLockSummaryDto(
        x.OperationId,
        x.Status,
        x.RequestedCount,
        x.Targets.Count(t => t.Status == "Locked"),
        x.Targets.Count(t => t.Status == "Skipped"),
        x.Targets.Count(t => t.Status == "Failed"),
        x.CompletedAtUtc))
    .SingleOrDefaultAsync(cancellationToken);

Review generated SQL and actual plan. AsNoTracking suits read projections. Avoid loading whole aggregates for screen data. Ensure target-status strings/enums match database mapping and indexes; include deterministic pagination for detail.

Never assume an ORM query is safe because it compiles. Look for client materialisation, N+1 dependency calls, unbounded Includes, functions on indexed predicates, implicit type conversion, missing tenant filter and unsupported SQL translation.

Review lab 7: frontend state and user truth

The initial React component:

function LockSelectedUsers({ selectedUsers }: Props) {
  const [loading, setLoading] = useState(false);

  async function lock() {
    setLoading(true);
    await fetch('/api/users/lock', {
      method: 'POST',
      body: JSON.stringify({ userIds: selectedUsers.map(x => x.id) })
    });
    setLoading(false);
    alert('Users locked');
  }

  return <button onClick={lock}>Lock selected</button>;
}

Review it as a state machine. It has no error, response validation, request ID, confirmation, selected/eligible distinction, retry/reconciliation, accessibility or route to operation status. It claims completion even when the server only accepted work.

Model visible states:

Selecting → Reviewing scope → Submitting → Accepted/monitoring
                         ↘ cancelled
Submitting → validation/forbidden/conflict/unknown delivery
Monitoring → completed / partial / failed / cancelled

The browser creates a request ID before submission and retains it until outcome reconciliation. A server-state layer/query cache can poll StatusUrl with appropriate backoff or use supported real-time updates. A lost response transitions to “Checking operation…” rather than immediately offering a second lock.

async function submitBulkLock(command: BulkLockCommand) {
  const response = await api.createBulkLock(command);
  navigate(response.statusUrl);
}

This concise handler assumes api maps error contracts and navigate leads to a durable operation route. The complexity belongs in named feature hooks/state, not a giant click handler.

Selection scope review

“Selected all” can mean visible page or all matching results. The confirmation must say which:

25 selected on this page
or
All 4,832 users matching “Project Cedar” selected, 3 excluded

If selection represents all matching results, the backend resolves a server-owned query snapshot/fingerprint under current authorisation. The client cannot safely send 4,832 arbitrary IDs and expect the count to remain meaningful after filters change.

Review lab 8: accessibility is observable behaviour

Review the confirmation dialog:

• real dialog semantics/accessible name/description;
• focus moves into it and returns to the trigger;
• tab/escape behaviour follows the supported component pattern;
• target scope, exclusion and irreversible consequence are text, not colour;
• reason field has label, hint and server/client validation;
• confirm button is disabled only with an explanation where needed;
• loading/retry/partial status is announced without constant noise;
• operation result remains available after a toast disappears;
• keyboard path works at zoom/narrow layout.

Review lab 9: tests are evidence, not decoration

Map risks to tests:

Test failure modes deliberately. A code review that only sees happy-path tests should ask where the system recovers from timeout, duplicate delivery, stale policy, partial provider outage and old/new deployment overlap.

Review lab 10: logging, audit and supportability

Bad logging:

logger.LogInformation("Locking users {@Request}", request);

This may store target IDs, reason, email-derived data or tokens depending on the object. Log a safe event:

logger.LogInformation(
    "Bulk lock operation {OperationId} accepted for organisation {OrganisationId} " +
    "with {RequestedCount} requested and {EligibleCount} eligible targets",
    operation.Id,
    operation.OrganisationId,
    operation.RequestedCount,
    operation.EligibleCount);

Keep high-cardinality operation IDs in access-controlled traces/logs, not metric labels. Metrics can measure accepted count, completion latency, provider failure category, target outcome counts and oldest queued operation. They should not label every user/organisation.

Audit is different from debug logging. Record actor, operation scope, reason, policy decision and outcome under access/retention controls. The audit should be durable and explain a privileged action without copying the entire user dataset into every event.

The review asks: When support says “operation 4f… is stuck,” can an authorised responder find current state, last provider result, retry history and safe next action? If the answer requires ad hoc database edits, the change is not operable.

Review lab 11: deployment and rollback

Review the rollout before approving code:

1. Add operation/target/outbox schema compatibly.
2. Deploy API/worker able to coexist with flag off.
3. Verify migration, health checks, telemetry and safe status route.
4. Enable for internal test organisation.
5. Run success, protected target, duplicate request, provider throttle and lost-response drills.
6. Monitor operation age, failure/skip rate and audit completeness.
7. Expand cohort with predefined stop conditions.
8. Disable new submissions if necessary; allow accepted operations to reconcile safely.
9. Remove temporary flag/schema paths only after evidence.

A release review comment

[Blocking before enablement] The migration and endpoint are compatible, but the plan has no stop signal or reconciliation behaviour after disabling the flag. If the provider begins throttling, accepted operations may be left ambiguous. Please document operation-age alert, pause-new-work behaviour, retry/dead-letter path and support runbook, then exercise it in the test tenant.

Review lab 12: prioritise findings

Not every comment deserves the same urgency.

If a risk cannot be fixed in this change, record accountable acceptance and a concrete follow-up. Do not bury it in a vague comment. Some risks—tenant isolation, privileged action audit, data corruption—should not be accepted casually.

Junior reviewer: I found ten issues. Should I write ten comments?

Senior reviewer: Group related findings. Start a design conversation if one cause produces many symptoms. The goal is a safer change, not a high comment count.

Review lab 13: author response and reviewer follow-through

Review is a dialogue. An effective author response links action/evidence:

Resolved in 8d31c:
- operation request now has organisation scope and RequestId
- unique (OrganisationId, RequestId) constraint added
- concurrent duplicate integration test added
- endpoint returns 202 + status URL

Open question: should protected target identities appear in UI or only count/reason?
Product/security decision requested in ADR-24 before enablement.

The reviewer verifies the evidence, not just that a comment is marked resolved. If a discussion changed scope, update acceptance/release plan. Praise thoughtful repair; this reinforces the team behaviour review is meant to create.

Do not repeatedly move goalposts. If a new serious issue appears after a change, explain why it was not visible earlier and adjust. Keep a review's scope proportional; a feature PR is not always the moment to rewrite unrelated architecture.

Review lab 14: a review timeline that respects flow

Before author begins, review outcome/risk. During implementation, invite a draft review for contract, schema or security choices. Before merge, review integrated code/tests/rollout. After deployment, check telemetry and feedback.

Refinement: policy + acceptance + risk map
Draft PR: API contract, operation schema, worker approach
Implementation review: code, tests, accessibility
Release review: migration, flag, monitoring, runbook
Post-release: metrics, incidents, follow-up removal

This is not bureaucracy when it replaces late surprise. Use it proportionately: a one-line CSS change does not require architecture review; a privileged bulk command does.

How to Leave Senior Review Feedback

Technical accuracy is only half the skill. Good feedback is specific, proportionate and open to context.

Use questions when the intent is unclear:

Could this endpoint be called twice after a client timeout? If so, what prevents
the loan application from being created twice?

Use direct language when the risk is clear:

This response returns the exception message to the client, which can expose internal
details. Please log the exception with the correlation ID and return the standard
Problem Details response.

Distinguish blocking issues from suggestions. Avoid flooding a pull request with repeated comments when one design-level conversation would be clearer. Recognise sound decisions as well as defects, and move complex disagreement into a short discussion rather than conducting architecture by fragmented comments.

The author owns the change, but the team owns the system. A senior reviewer should be willing to block a real correctness, security or operability risk—and equally willing to let a harmless personal preference go.

The Senior Review Checklist

Build a review process people can use

A checklist is useful only when it changes the conversation. Teams often make review difficult in two opposite ways: every pull request is enormous and impossible to understand, or every tiny change receives a ritual set of comments that adds no safety. Senior engineers improve the system around the review.

Make a change reviewable before it is clever

Authors can make a review dramatically more effective with a short description:

Outcome: allow organisation administrators to lock selected accounts.
Risk: privileged cross-tenant operation; provider outage; duplicate submission.
Design: durable operation + worker, policy checked on server, feature flag.
Evidence: integration tests for tenant/policy/idempotency; browser test for dialog.
Rollout: internal tenant first; alert on oldest operation age; runbook linked.
Out of scope: self-service unlock and bulk CSV import.

This is not a request for an essay. It is the minimum map a reviewer needs to distinguish intentional trade-offs from accidental omissions. Link the requirement, design record, incident or issue when one exists. Include screenshots or a short recording for meaningful UI changes, and show migration SQL or execution evidence when a data change is central.

Keep commits and pull requests coherent. A formatter-only reformat across 400 files hides the two lines that alter authorisation. Split mechanical changes from behaviour changes where practical. If they must travel together, explain it and give reviewers an order: contract first, then model, migration, worker, UI and tests.

Junior developer: Is it bad if my pull request is large?

Senior reviewer: Size is a signal, not a verdict. A large, well-mapped migration may be safer than five disconnected pull requests. But if I cannot hold the behaviour in my head, we should find a seam: introduce a compatible schema, ship the reader, then enable the writer later.

Review in passes, not as one long scroll

Reading linearly from the first changed file invites local comments before you understand the intent. Use deliberate passes instead:

1. Read the problem statement and expected user outcome.
2. Trace public contracts: route, request, response, events and schema.
3. Identify trust boundaries: identity, organisation, permissions, external input and secrets.
4. Trace state and failure handling across API, database, worker and UI.
5. Inspect the highest-risk implementation detail and generated SQL/plan where relevant.
6. Read tests as claims about behaviour; look for the most expensive failure modes.
7. Review deployment, observability and rollback for changes with operational impact.
8. Leave comments after you can rank their consequence.

Use tools as evidence, not as substitutes for thought

Static analysis, formatters, linters, dependency scanners and test suites remove routine work. Configure them to run early and consistently. A reviewer should not spend attention asking for trailing whitespace or a missing nullability annotation that tooling can enforce.

But tools answer bounded questions. A scanner can report a vulnerable package; it cannot decide whether a proposed upgrade breaks an API contract or whether the package is reachable. A test suite can say the selected assertions passed; it cannot prove the assertions reflect the customer outcome. A clean compilation is necessary evidence, not sufficient evidence.

When a tool reports something, translate it into the system question:

Review boundaries and architectural ownership

Many difficult reviews are not about a bad if statement. They expose a boundary that has become unclear. Ask who owns a decision and where it can be enforced.

Validate at every boundary for its purpose

The browser may validate a required field to give instant feedback. The API must validate the same business constraint because clients can bypass the browser. The database may need a unique constraint because concurrent API requests can bypass application-level checks. These are complementary protections, not duplicated mistakes.

For a value such as an account status, separate concerns:

Browser: is the input complete and understandable?
API: is the caller authenticated, authorised and making a valid command?
Domain: does this state transition obey business policy?
Database: can concurrent writes violate the invariant?
Worker/provider: can the external side effect be retried safely?

In review, reject the argument “the UI already prevents it” when the consequence matters. Conversely, do not force a UI component to duplicate complicated domain logic that the API can expose as a clear capability or validation response.

Keep policy visible

Permission checks hidden inside a repository method are hard to audit; permissions derived only from a disabled button are unsafe. Prefer an explicit policy/capability decision close to the command boundary and tests that name it. If rules depend on hierarchy, ownership or geography, make the relevant data and decision visible in the model.

var decision = lockPolicy.CanLock(actor, targetAccount, organisation);
if (!decision.Allowed)
{
    return Results.Forbid();
}

The exact abstraction varies. What matters is that a reviewer can answer: who acted, on what resource, under which tenant/context, according to which rule, and how is the decision tested?

Avoid accidental distributed transactions

An API that writes a row, calls a payment or identity provider and then writes another row has already crossed failure domains. A transaction cannot generally make the external call atomic. Review for durable intent, an outbox or work item, idempotency keys, compensation and a supportable terminal state.

This does not mean every API needs a queue. For a low-value notification, a synchronous best-effort call with explicit failure behaviour may be appropriate. The review question is whether the chosen consistency model matches the customer and business consequence.

Junior developer: Why not retry every failure three times?

Senior reviewer: Because retries repeat actions. First decide whether the operation is safe to repeat, which errors are transient, who owns the deadline and what happens after attempts are exhausted. A retry without idempotency can multiply the original incident.

Review C# with runtime behaviour in mind

Readable C# is valuable, but runtime semantics deserve equal attention. These questions catch issues that compile cleanly.

Lifetimes, disposal and cancellation

Check that scoped services do not leak into singleton services, IDisposable resources are owned by the container or disposed deliberately, and cancellation flows from request/worker shutdown through I/O calls. Do not pass a request cancellation token to work that must outlive the request; give durable work its own lifecycle.

public sealed class ReportController : ControllerBase
{
    [HttpGet("reports/{id:guid}")]
    public async Task<IActionResult> Download(Guid id, CancellationToken cancellationToken)
    {
        var result = await _reports.GetAsync(id, cancellationToken);
        return result is null ? NotFound() : File(result.Content, result.ContentType);
    }
}

Ask what cancels this operation, whether cancellation leaves state consistent, and whether a timeout is enforced at the right level. CancellationToken is not a magic timeout; it only matters if downstream work observes it.

Async correctness and concurrency

Look for fire-and-forget tasks, blocking .Result/.Wait(), lost exceptions, unbounded parallelism, shared mutable state and missing concurrency control. A loop with Task.WhenAll over an unbounded request list may overload SQL Server or an external provider. Bound concurrency and make partial failure explicit.

using var gate = new SemaphoreSlim(8);
var tasks = items.Select(async item =>
{
    await gate.WaitAsync(cancellationToken);
    try { await ProcessAsync(item, cancellationToken); }
    finally { gate.Release(); }
});

await Task.WhenAll(tasks);

Eight is not universally correct. It is a chosen limit that should be measured against provider limits, connection pools, workload type and fairness. Good review asks why a value exists and what metric would tell the team to change it.

Model errors intentionally

Avoid using exceptions for expected validation or not-found outcomes, and avoid collapsing every exception into an identical 500. Review error mapping for stable client semantics, safe messages and internal diagnostic correlation.

return outcome switch
{
    LockOutcome.Accepted accepted => AcceptedAtRoute("operation", new { id = accepted.Id }, accepted),
    LockOutcome.Forbidden => Forbid(),
    LockOutcome.NotFound => NotFound(),
    LockOutcome.Invalid invalid => ValidationProblem(invalid.Errors),
    _ => Problem(statusCode: StatusCodes.Status500InternalServerError)
};

The UI needs distinguishable outcomes to guide the user. Support needs a correlation ID and structured details. Attackers should not receive stack traces, SQL fragments or internal topology.

C# and ASP.NET Core

• Does the implementation satisfy the business requirement and failure paths?
• Are responsibilities and domain invariants in the right place?
• Are async, cancellation, DI lifetimes and disposal correct?
• Does EF Core perform filtering, projection and pagination in SQL?
• Are errors consistent, logs structured and sensitive data protected?
• Do tests defend important behaviour, permissions and edge cases?

SQL and Data Access

• Is the query bounded and shaped for production volume?
• Are predicates SARGable and parameter types aligned?
• Do joins return the intended cardinality?
• Do indexes support the measured workload without excessive write cost?
• Have actual plans, logical reads, blocking and plan stability been considered?
• Is the migration backward-compatible and safe to deploy?

SPA Frontend

• Are loading, error, empty and success states handled?
• Is state owned at the appropriate level?
• Are API calls, cancellation and cache invalidation consistent?
• Are lists bounded, keys stable and forms resilient?
• Is authorisation enforced by the API as well as represented in the UI?
• Are semantic HTML, keyboard access and understandable errors included?

Review SQL and EF Core as a production workload

Data-access code is easy to approve when it runs against a nearly empty local database. A senior review asks what happens with real cardinality, skewed parameters, concurrent activity and a deployment in progress.

Read the query shape, not just the LINQ

LINQ expresses intent, but the database executes translated SQL. Review whether filtering, ordering, projection and pagination happen before materialisation. Prefer a narrow projection over loading an entity graph merely to return three fields.

var page = await db.Accounts
    .Where(x => x.OrganisationId == organisationId && x.Status == AccountStatus.Active)
    .OrderBy(x => x.Id)
    .Select(x => new AccountSummary(x.Id, x.DisplayName, x.LastSeenUtc))
    .Take(pageSize)
    .ToListAsync(cancellationToken);

Ask what makes ordering stable, whether a user can request an excessive page size, and whether OrganisationId, status and the sort key deserve an index for the measured workload. Offset pagination can become slow at deep pages; a keyset/cursor approach may be a better fit where users traverse large ordered results. Do not cargo-cult either approach—review the actual product navigation.

When a change matters, inspect generated SQL and run a representative query plan against production-shaped, safely anonymised or generated data. Estimates, scans, logical reads, duration and blocking are evidence. A new index can accelerate a read and add cost to every write, so name the workload it supports.

Treat migrations as software releases

Schema changes require a compatibility story. Adding a nullable column is generally easier to roll out than adding a non-null column with an application-only default. Renaming a column may need a period where old and new code can coexist. Dropping a column should follow evidence that all readers and writers are gone.

Expand: add nullable NewStatus and deploy code that reads old/new safely.
Backfill: populate in controlled batches with monitoring.
Switch: deploy writers/readers that rely on NewStatus.
Contract: remove OldStatus after the compatibility window and verification.

Review locks, table size, online/offline capabilities, transaction log impact, rollback and backup/restore expectations. A migration that is syntactically valid can still be dangerous because it blocks a busy table for too long. Require rehearsal and timing evidence for high-risk tables.

Consider consistency deliberately

Two authors can read the same row and each submit a valid update. If only one should win, review for optimistic concurrency tokens, atomic update predicates or another explicit control. A last-write-wins outcome is sometimes correct, but it should be a product decision rather than a race that happened to be untested.

var updated = await db.Accounts
    .Where(x => x.Id == id && x.Version == request.Version)
    .ExecuteUpdateAsync(s => s
        .SetProperty(x => x.Status, AccountStatus.Locked)
        .SetProperty(x => x.Version, x => x.Version + 1), cancellationToken);

if (updated == 0)
    return Conflict(new { message = "The account changed; refresh and try again." });

The reviewer should ask how the client obtains the version, what it can do after conflict, and whether a privileged action needs a server-side fresh policy check regardless of the version.

Review SPA code as a stateful client

The frontend is not a collection of isolated components. It is a long-lived client operating on an unreliable network, with stale data, navigation, accessibility and user intent to manage.

Make state transitions explicit

For a remote operation, distinguish idle, submitting, accepted/pending, partial result, completed and failed states. A single isLoading Boolean cannot communicate all of them. Review whether a user can double-click, navigate away, retry safely, or understand that a 202 Accepted operation is still running.

type BulkLockState =
  | { kind: 'idle' }
  | { kind: 'submitting' }
  | { kind: 'tracking'; operationId: string }
  | { kind: 'complete'; locked: number; skipped: number }
  | { kind: 'error'; message: string };

State should live at the lowest level that has a single clear owner, but no lower. If a selected-account list affects a toolbar, table and confirmation dialog, pushing it through multiple component layers invites divergent copies. Use a feature-level store/service or a well-defined parent owner; avoid global state merely because it is available.

Check network and cache behaviour

Review loading and error states for the initial view, refresh, empty result, slow request and cancellation. When a mutation succeeds, which cached queries become stale? A UI that shows “locked” while a refreshed list shows “active” has not just a cosmetic bug; it has lost user trust.

Give requests a cancellation or freshness strategy where navigation and rapid filter changes make stale responses likely. Ensure error messages tell the user what happened and a sensible next action without exposing internal details.

Accessibility is part of the acceptance criteria

Review semantic controls before custom event handlers. A communicates role, focus and keyboard behaviour that a clickable

Bad: You clearly do not understand async programming.

Useful: This task is started without awaiting or retaining it. If it faults after the
request ends, the exception is unobserved and the caller receives success. What
outcome should the customer see, and should this instead become durable background work?

I cannot see a tenant predicate in this query or a database policy that supplies one.
If this endpoint is called with an ID from another organisation, it appears to return
that record. Please point me to the boundary I missed, or add a tenant-scoped query
and integration test before merge.

[HttpPost("accounts/{id}/reset-password")]
public async Task<IActionResult> Reset(Guid id)
{
    var account = await _db.Accounts.FindAsync(id);
    account.PasswordHash = _passwords.CreateTemporaryPassword();
    await _db.SaveChangesAsync();
    await _email.SendResetAsync(account.Email);
    return Ok();
}

This could fail when the table contains 100,000 records.
This state belongs higher because two child components depend on it.
This query filters in memory rather than in SQL Server.
This endpoint creates a resource, so 201 Created communicates the contract better.
This route guard improves UX, but the API must still enforce authorisation.
This Include graph may multiply rows; a projection would be safer.