Talkwise Warden - Code Review Guidelines

talkwise-warden-code-review.md

Talkwise Warden - Code Review Guidelines

This document outlines the code review standards for talkwise-warden based on patterns observed in past pull requests. Use this as a checklist when reviewing PRs or writing new code.

🔴 Critical Issues (Must Fix)

1. Always Use the GitHub API Client

❌ Don't:

const response = await axios.get(
  `https://api.github.com/repos/${owner}/${repo}/issues`,
  {
    headers: {
      Authorization: `Bearer ${accessToken}`,
      Accept: 'application/vnd.github+json'
    }
  }
);

✅ Do:

import { createGitHubApiClient } from '@utils/github-api-client';

const http = createGitHubApiClient();
const response = await http.get(`/repos/${owner}/${repo}/issues`);

Why: Centralized configuration, consistency, and maintainability. See Controllers CLAUDE.md for details.

2. Never Use any Type

❌ Don't:

const whereClause: any = {
  repositoryId: repositoryIds,
};

✅ Do:

interface WhereClause {
  repositoryId: string[];
  pullRequest?: {
    state: string;
  };
}

const whereClause: WhereClause = {
  repositoryId: repositoryIds,
};

Why: Type safety is one of TypeScript's core benefits. Using any defeats the purpose and makes refactoring dangerous.

3. Use HTTP Status Code Constants

❌ Don't:

throw new AppError('Commit message is required', 400);
res.status(200).json({ success: true });

✅ Do:

import { StatusCodes } from 'http-status-codes';

throw new AppError('Commit message is required', StatusCodes.BAD_REQUEST);
res.status(StatusCodes.OK).json({ success: true });

Why: Makes code self-documenting, prevents typos (e.g., 409 vs 499), and improves searchability.

🟡 Important Issues (Should Fix)

4. Extract Magic Numbers to Constants

❌ Don't:

const maxSequence = Math.min(expectedSequence, 1000000);
const maxPages = 10; // Limit: 1000 issues max
hasMore = response.data.length === 100;

✅ Do:

const MAX_SEQUENCE_GAP_LIMIT = 1000000;
const MAX_PAGINATION_PAGES = 10;
const GITHUB_MAX_PER_PAGE = 100;

const maxSequence = Math.min(expectedSequence, MAX_SEQUENCE_GAP_LIMIT);
const maxPages = MAX_PAGINATION_PAGES;
hasMore = response.data.length === GITHUB_MAX_PER_PAGE;

Why: Makes intent clear, prevents errors when values need to change, enables easy configuration.

5. Use Object Property Shorthand

❌ Don't:

whereClause.pullRequest = {
  state: state,
};

✅ Do:

whereClause.pullRequest = {
  state
};

Why: Cleaner, more concise, modern JavaScript standard.

6. Simplify Complex Conditional Logic

❌ Don't:

const repo = repoParam.replace(/\.git$/i, '');
[projectId.match(/^[0-9a-f-]{36}$/i) ? 'id' : 'slug']: projectId

✅ Do:

// Create helper functions
function removeGitSuffix(repo: string): string {
  return repo.replace(/\.git$/i, '');
}

function isUuid(value: string): boolean {
  return /^[0-9a-f-]{36}$/i.test(value);
}

function isSlug(value: string): boolean {
  return !isUuid(value);
}

// Use them
const repo = removeGitSuffix(repoParam);
[isSlug(projectId) ? 'slug' : 'id']: projectId

Why: Improves readability, makes logic testable, enables reuse.

7. Prefer Optional Chaining Over Try-Catch for Property Access

❌ Don't:

const actualIssues = issues.filter((issue: Record<string, unknown>) => {
  try {
    return !issue.pull_request;
  } catch (e) {
    logger.warn({ error: e instanceof Error ? e.message : String(e) });
    return false;
  }
});

✅ Do:

const actualIssues = issues.filter((issue: Record<string, unknown>) => {
  return !issue?.pull_request;
});

Why: Simpler, more idiomatic JavaScript. Property access doesn't throw errors in JavaScript—it returns undefined.

8. Remove Useless Assignments Before break

❌ Don't:

if (!response || !response.data) {
  hasMore = false;  // Useless assignment
  break;
}

✅ Do:

if (!response || !response.data) {
  break;
}

Why: The assignment has no effect since the loop exits immediately.

🟢 Style & Best Practices

9. Token Validation Should Happen at Startup

❌ Don't:

const accessToken = process.env.GITHUB_ACCESS_TOKEN;
if (!accessToken) {
  throw new AppError('GitHub API token not configured', 500);
}

✅ Do: Ensure getGithubAccessToken() validates at app startup (already implemented in warden). Remove runtime checks in individual functions.

Why: Fail fast at startup rather than during user requests.

10. Keep Destructuring Outside Try-Catch

This is a judgment call. If there's a risk that req.params could be undefined or malformed:

Consider:

async reconcileRepository = async (req: Request, res: Response, next: NextFunction): Promise<void> => {
  const { repositoryId } = req.params;
  try {
    // implementation
  } catch (error) {
    next(error);
  }
};

Why: Simpler code. If repositoryId is undefined, TypeScript/validation middleware should catch it earlier.

11. Move Type Augmentations to Global Type Files

❌ Don't:

// Inside middleware file
declare module 'express-serve-static-core' {
  interface Request {
    requestId?: string;
    logger?: pino.Logger;
  }
}

✅ Do: Create src/types/global.d.ts or similar for type augmentations.

Why: Keeps type declarations organized and easily discoverable.

12. Consider More Specific Naming

Example:

• requestContextMiddleware → requestLoggerMiddleware or requestContextLoggerMiddleware

Why: "Context" is vague. More specific names clarify intent.

13. Validate External API Responses

When dealing with GitHub API or other external APIs that may return unexpected data:

// Validate response structure
if (!response || !response.data) {
  logger.warn({ page, repositoryKey }, 'Invalid response structure from GitHub API');
  break;
}

const issues = Array.isArray(response.data) ? response.data : [];
if (!Array.isArray(issues)) {
  logger.warn({ page, repositoryKey }, 'Issues response is not an array');
  break;
}

Why: External APIs can change or fail in unexpected ways. Defensive programming prevents crashes.

📋 Code Review Checklist

When reviewing a PR, check for:

• GitHub API calls use createGitHubApiClient()
• No usage of any type (use proper types/interfaces)
• HTTP status codes use StatusCodes enum
• Magic numbers extracted to constants
• Object property shorthand used where applicable
• Complex logic extracted to helper functions
• Optional chaining used instead of unnecessary try-catch
• No useless assignments before break/return
• Type augmentations in proper type definition files
• Middleware and functions have clear, specific names
• External API responses validated
• Consistent formatting (handled by Prettier)

🔍 Common Patterns to Watch For

GitHub API Pagination

const GITHUB_MAX_PER_PAGE = 100;
const MAX_PAGINATION_PAGES = 10;

let page = 1;
let hasMore = true;

while (hasMore && page <= MAX_PAGINATION_PAGES) {
  const http = createGitHubApiClient();
  const response = await http.get(`/repos/${owner}/${repo}/issues`, {
    params: {
      state: 'all',
      per_page: GITHUB_MAX_PER_PAGE,
      page
    }
  });

  const items = response.data;
  hasMore = items.length === GITHUB_MAX_PER_PAGE;
  page++;
}

Repository URL Parsing

function parseRepoUrl(url: string): { owner: string; repo: string } | null {
  const match = url.match(/github\.com\/([^/]+)\/([^/]+)/);
  if (!match) {
    return null;
  }
  const [, owner, repoWithGit] = match;
  const repo = removeGitSuffix(repoWithGit);
  return { owner, repo };
}

Standard Error Response

catch (error: unknown) {
  if (axios.isAxiosError(error)) {
    const status = error.response?.status || StatusCodes.INTERNAL_SERVER_ERROR;
    const message = error.response?.data?.message || error.message;
    throw new AppError(message, status);
  }
  throw error;
}

📚 Related Documentation

• Project README
• Model Guidelines
• Controller Guidelines

🤝 Philosophy

Code reviews are about:

1. Correctness - Does it work and handle edge cases?
2. Maintainability - Can future developers understand and modify it?
3. Consistency - Does it follow established patterns?
4. Type Safety - Does it leverage TypeScript properly?

Be thorough but constructive. The goal is to ship high-quality code while helping each other grow as engineers.