Skip to content

Buổi 15: AI Code Review Tự Động Trên PR

Mục Tiêu Buổi Học

Sinh viên sẽ xây dựng GitHub Action tự động review code trên PR, phát hiện code smells (magic numbers, bad naming, DRY violations) và bắt buộc dev fix bằng evidence — không phải chỉ đồng ý.


Phần 1: Lý Thuyết (20 phút)

cm-code-review: Full Review Lifecycle (3 Parts)

Code review không phải task một chiều. Nó là interactive workflow với 3 phases rõ ràng:

Part A: Request Review

  • Dev hoàn thành feature → push PR
  • Gọi reviewer (AI hoặc human)
  • Reviewer phải check đầu vào: có test không, architecture có hợp lý không

Part B: Receive Review

  • Reviewer tìm issues: bugs, code smells, violations
  • Post comments trực tiếp trên PR với evidence (line number, code snippet, reason)
  • Dev phải respond — không phải chỉ đồng ý mà phải verify

Part C: Finish Branch

  • Dev fix issues
  • Dev re-request review
  • Reviewer approve
  • Merge PR
  • Clean branch

Diagram Lifecycle:

┌─────────────────┐
│  Dev push PR    │
└────────┬────────┘

    Part A: Request


┌─────────────────┐
│  AI Review      │
│  Post comments  │
└────────┬────────┘

    Part B: Receive


┌─────────────────┐
│  Dev responds   │
│  with evidence  │
└────────┬────────┘

    Part C: Finish


┌─────────────────┐
│  Approve +      │
│  Merge          │
└─────────────────┘

Anti-Pattern: Performative Agreement ❌

BAD RESPONSE (Performative Agreement):

Reviewer: "This validates email without checking for SQL injection."

Dev: "Good catch! Fixed." ❌

Why bad?

  • Dev didn't actually verify
  • Might not have fixed correctly
  • Creates false sense of security
  • No learning happens

GOOD RESPONSE (Evidence-Based):

Reviewer: "This validates email without checking for SQL injection."

Dev: "I verified this: [evidence]" ✅
- Added SQLite parameterized query (line 47)
- Changed from: user.insert(email) 
- To: db.prepare('INSERT INTO users (email) VALUES (?)').run(email)
- The suggestion is correct because SQL injection on email field could expose user_id column
- Added unit test: test/security/sql-injection.test.js
- Fixed." ✅

Why good?

  • Dev showed they understand the problem
  • Dev verified the fix with evidence
  • Dev explained the risk
  • Dev added test coverage
  • Creates learning opportunity

Response Framework: 3 Types

Khi reviewer comment, dev respond theo framework:

SituationResponse
Technically CorrectFix it immediately. Acknowledge the issue.
Unclear IntentAsk clarification: "Do you mean...? Can you point to example?"
Technically QuestionableChallenge with evidence: "I think this is safe because... Can you explain the risk?"
Stylistic PreferenceDiscuss trade-offs: "This is more readable but slower by 5%. Trade-off worth it?"

cm-clean-code: 7-Point Hygiene Checklist

AI reviewer will scan for:

1. Dead Code → DELETE don't comment

javascript
// ❌ BAD
// const oldHandler = (req, res) => { ... }  // Not used anymore

// ✅ GOOD
// Removed oldHandler. Logic moved to newHandler (commit xyz)

2. Unused Imports → REMOVE

javascript
// ❌ BAD
import axios from 'axios';
import { readFile } from 'fs';
import moment from 'moment';

export const getUser = (id) => {
  // Only uses fetch, never uses axios, readFile, or moment
}

// ✅ GOOD
import { readFile } from 'fs';

export const getUser = (id) => {
  // Removed unused axios, moment
}

3. Magic Numbers & Strings → EXTRACT to named const

javascript
// ❌ BAD
if (email.length > 255) throw new Error('Email too long');
if (password.length < 8) throw new Error('Password too short');
if (attempts > 5) lockAccount();

// ✅ GOOD
const VALIDATION = {
  EMAIL_MAX_LENGTH: 255,
  PASSWORD_MIN_LENGTH: 8,
  LOGIN_MAX_ATTEMPTS: 5
};

if (email.length > VALIDATION.EMAIL_MAX_LENGTH) throw new Error(...);
if (password.length < VALIDATION.PASSWORD_MIN_LENGTH) throw new Error(...);
if (attempts > VALIDATION.LOGIN_MAX_ATTEMPTS) lockAccount();

4. Naming → RENAME to describe intent

javascript
// ❌ BAD
const x = userData.filter(u => u.age > 18);
const p = x.map(u => u.password);

// ✅ GOOD
const adultsOnly = userData.filter(user => user.age > 18);
const passwordsOfAdults = adultsOnly.map(user => user.password);

5. Single Responsibility Principle (SRP) → EXTRACT into separate units

javascript
// ❌ BAD — validateUser does 4 things
const validateUser = (user) => {
  // 1. Check email format
  // 2. Query database
  // 3. Log to file
  // 4. Send email notification
}

// ✅ GOOD — Separate concerns
const isValidEmail = (email) => { /* check format */ };
const userExists = (email) => { /* query database */ };
const logValidation = (email, result) => { /* log to file */ };
const notifyUser = (email) => { /* send email */ };

6. DRY — EXTRACT shared logic

javascript
// ❌ BAD — Duplicate validation in 3 routes
app.post('/register', (req, res) => {
  if (!req.body.email) return res.status(400).json({ error: 'Email required' });
  if (req.body.email.length > 255) return res.status(400).json({ error: 'Email too long' });
  // ...
});

app.put('/users/:id', (req, res) => {
  if (!req.body.email) return res.status(400).json({ error: 'Email required' });
  if (req.body.email.length > 255) return res.status(400).json({ error: 'Email too long' });
  // ...
});

// ✅ GOOD — Extract to middleware
const validateEmail = (req, res, next) => {
  if (!req.body.email) return res.status(400).json({ error: 'Email required' });
  if (req.body.email.length > 255) return res.status(400).json({ error: 'Email too long' });
  next();
};

app.post('/register', validateEmail, (req, res) => { /* ... */ });
app.put('/users/:id', validateEmail, (req, res) => { /* ... */ });

7. Nesting Depth > 3 → EXTRACT, early return, guard clauses

javascript
// ❌ BAD — 5 levels of nesting
function processOrder(order) {
  if (order) {
    if (order.items) {
      if (order.items.length > 0) {
        if (order.customer) {
          if (order.customer.verified) {
            // Finally do real work at depth 5
          }
        }
      }
    }
  }
}

// ✅ GOOD — Guard clauses, early return
function processOrder(order) {
  if (!order) return;
  if (!order.items || order.items.length === 0) return;
  if (!order.customer || !order.customer.verified) return;
  
  // Real logic at depth 1
  chargeCustomer(order);
  shipItems(order.items);
}

Phần 2: Thực Hành (60 phút)

Task 1: Tạo GitHub Action Cho AI Review

File: .github/workflows/ai-code-review.yml

yaml
name: AI Code Review

on:
  pull_request:
    types: [opened, synchronize]

jobs:
  review:
    runs-on: ubuntu-latest
    permissions:
      pull-requests: write
      contents: read

    steps:
      - uses: actions/checkout@v3
        with:
          fetch-depth: 0

      - name: Setup Node.js
        uses: actions/setup-node@v3
        with:
          node-version: '18'

      - name: Install dependencies
        run: npm install

      - name: Get changed files
        id: changed-files
        uses: tj-actions/changed-files@v44
        with:
          files: |
            src/**/*.js
            !src/**/*.test.js

      - name: Run AI Code Review
        if: steps.changed-files.outputs.any_changed == 'true'
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
          PR_NUMBER: ${{ github.event.pull_request.number }}
        run: |
          node scripts/code-review.js \
            --files="${{ steps.changed-files.outputs.all_changed_files }}" \
            --pr=$PR_NUMBER

Task 2: Viết scripts/code-review.js

File này sẽ scan code violations từ 7-point checklist → post review comments trên PR.

Tạo file: scripts/code-review.js

javascript
const fs = require('fs');
const path = require('path');
const https = require('https');

/**
 * AI Code Review for TaskFlow QA
 * Scans for 7-point clean code violations
 * Posts review comments to GitHub PR
 * 
 * Usage: node scripts/code-review.js --files="src/api.js src/db.js" --pr=42
 */

class CodeReviewer {
  constructor(config = {}) {
    this.files = (config.files || '').split(' ').filter(Boolean);
    this.prNumber = config.pr;
    this.githubToken = process.env.GITHUB_TOKEN;
    this.githubOwner = process.env.GITHUB_REPOSITORY_OWNER || 'your-org';
    this.githubRepo = process.env.GITHUB_REPOSITORY?.split('/')[1] || 'taskflow';
    this.comments = [];
    this.violations = {
      deadCode: 0,
      unusedImports: 0,
      magicNumbers: 0,
      badNaming: 0,
      srpViolation: 0,
      dryViolation: 0,
      nestingDepth: 0
    };
  }

  /**
   * Check 1: Dead Code — Comments that are NOT meaningful
   */
  checkDeadCode(file, content) {
    const lines = content.split('\n');
    
    lines.forEach((line, idx) => {
      const lineNum = idx + 1;
      
      // Pattern: const x = ... // Not used
      if (/^\s*\/\/\s*(const|let|var|function|class)/.test(line)) {
        this.comments.push({
          path: file,
          line: lineNum,
          side: 'RIGHT',
          body: `🧹 **Dead Code** (Point 1)
This looks like commented-out code. Please delete it instead of commenting.
If you need to preserve history, git log has it.

✅ What to do:
- Delete the commented line(s)
- Or add a comment explaining WHY (e.g., "// TODO: remove after migration")

${line.trim()}`
        });
        this.violations.deadCode++;
      }
    });
  }

  /**
   * Check 2: Unused Imports
   */
  checkUnusedImports(file, content) {
    const lines = content.split('\n');
    const imports = new Map(); // { name: lineNum }
    const usage = new Set(); // { name }

    // Find all imports
    lines.forEach((line, idx) => {
      const importMatch = line.match(/import\s+(?:{([\s\S]+?)}\s+from|(\w+)\s+from)/);
      if (importMatch) {
        const imported = importMatch[1] || importMatch[2];
        if (importMatch[1]) {
          // Named imports: import { a, b } from 'x'
          imported.split(',').forEach(name => {
            const cleanName = name.trim().split(' as ')[0];
            imports.set(cleanName, idx + 1);
          });
        } else {
          // Default import: import a from 'x'
          imports.set(imported, idx + 1);
        }
      }
    });

    // Find usage of imports
    const importedNames = Array.from(imports.keys());
    importedNames.forEach(name => {
      const importLine = imports.get(name);
      // Count usage outside import line
      const usageCount = lines.reduce((count, line, idx) => {
        if (idx + 1 === importLine) return count; // Skip import line itself
        return count + (line.includes(name) ? 1 : 0);
      }, 0);

      if (usageCount === 0) {
        this.comments.push({
          path: file,
          line: imports.get(name),
          side: 'RIGHT',
          body: `🧹 **Unused Import** (Point 2)
\`${name}\` is imported but never used.

✅ Remove this import:
\`\`\`diff
- import { ${name} } from '...'
\`\`\``
        });
        this.violations.unusedImports++;
      }
    });
  }

  /**
   * Check 3: Magic Numbers & Strings
   */
  checkMagicNumbers(file, content) {
    const lines = content.split('\n');
    
    // Pattern: if (x > 255) or if (attempts > 5) etc
    const magicPatterns = [
      { pattern: />\s*(\d{2,})/, type: 'number' }, // > 100, > 255
      { pattern: /<\s*(\d+)/, type: 'number' },     // < 10
      { pattern: /(===|==)\s*['"](\w+)['"]/, type: 'string' } // == 'status'
    ];

    lines.forEach((line, idx) => {
      magicPatterns.forEach(({ pattern, type }) => {
        if (pattern.test(line) && line.includes('if') || line.includes('while')) {
          const magic = line.match(pattern)?.[1] || line.match(pattern)?.[0];
          
          if (type === 'number' && magic && magic.length > 1) {
            this.comments.push({
              path: file,
              line: idx + 1,
              side: 'RIGHT',
              body: `✨ **Magic Number** (Point 3)
The number \`${magic}\` should be extracted to a named constant.

✅ What to do:
At top of file, create:
\`\`\`javascript
const LIMITS = {
  EMAIL_MAX_LENGTH: 255,
  PASSWORD_MIN_LENGTH: 8,
  LOGIN_MAX_ATTEMPTS: 5
};
\`\`\`

Then use:
\`\`\`javascript
if (email.length > LIMITS.EMAIL_MAX_LENGTH) { ... }
\`\`\``
            });
            this.violations.magicNumbers++;
          }
        }
      });
    });
  }

  /**
   * Check 4: Bad Variable Naming
   */
  checkBadNaming(file, content) {
    const lines = content.split('\n');
    
    // Single letter variables (except in loops)
    lines.forEach((line, idx) => {
      // Pattern: const x = or let d = or var p =
      const badNameMatch = line.match(/(?:const|let|var)\s+([a-z])\s*=/);
      
      if (badNameMatch && !line.includes('for')) {
        const varName = badNameMatch[1];
        this.comments.push({
          path: file,
          line: idx + 1,
          side: 'RIGHT',
          body: `📝 **Poor Variable Name** (Point 4)
Variable \`${varName}\` is too short. What does it represent?

✅ Better names:
- \`${varName}\`\`users\` or \`data\` or \`timestamp\`?
- Use: \`const ${varName}Data = ...\` or \`const ${varName}List = ...\`
- Describe the **intent**, not the **type**`
        });
        this.violations.badNaming++;
      }
    });
  }

  /**
   * Check 5: Single Responsibility Principle
   */
  checkSingleResponsibility(file, content) {
    const lines = content.split('\n');
    
    // Look for functions that do too many things
    // Pattern: long function with multiple unrelated operations
    lines.forEach((line, idx) => {
      if (line.includes('const ') && line.includes('=') && line.includes('=>')) {
        // Get function body
        let functionStart = idx;
        let braceCount = 0;
        let functionBody = '';
        
        for (let i = idx; i < Math.min(idx + 20, lines.length); i++) {
          functionBody += lines[i];
          braceCount += (lines[i].match(/{/g) || []).length;
          braceCount -= (lines[i].match(/}/g) || []).length;
          
          if (braceCount === 0 && i > idx) break;
        }

        // Check for multiple operations (heuristic: many if/else, many function calls)
        const ifCount = (functionBody.match(/if\s*\(/g) || []).length;
        const callCount = (functionBody.match(/\.\w+\(/g) || []).length;

        if (ifCount > 3 && callCount > 3 && functionBody.length > 500) {
          this.comments.push({
            path: file,
            line: idx + 1,
            side: 'RIGHT',
            body: `🎯 **SRP Violation** (Point 5)
This function seems to do multiple things (${ifCount} conditionals, ${callCount} calls). Extract responsibilities.

✅ What to do:
1. **Identify responsibilities**: validation, database query, logging, response
2. **Extract**: Create separate functions
3. **Reuse**: Compose small functions

Example:
\`\`\`javascript
// Before: validateAndSave does 4 things
const validateAndSave = (user) => { /* 30 lines */ };

// After: Each function has ONE job
const isValidUser = (user) => { /* 5 lines */ };
const saveUser = (user) => { /* 3 lines */ };
const logUser = (user) => { /* 3 lines */ };
const notifyUser = (user) => { /* 3 lines */ };
\`\`\``
          });
          this.violations.srpViolation++;
        }
      }
    });
  }

  /**
   * Check 6: DRY — Don't Repeat Yourself
   */
  checkDRY(file, content) {
    const lines = content.split('\n');
    const codeBlocks = [];

    // Extract meaningful code lines (skip comments, blanks)
    lines.forEach((line, idx) => {
      const clean = line.trim();
      if (clean && !clean.startsWith('//') && clean.length > 20) {
        codeBlocks.push({ line: clean, num: idx + 1 });
      }
    });

    // Find duplicates
    for (let i = 0; i < codeBlocks.length; i++) {
      for (let j = i + 1; j < codeBlocks.length; j++) {
        const similarity = this.stringSimilarity(codeBlocks[i].line, codeBlocks[j].line);
        if (similarity > 0.85) { // 85% similar
          this.comments.push({
            path: file,
            line: codeBlocks[j].num,
            side: 'RIGHT',
            body: `🔄 **DRY Violation** (Point 6)
This code is similar to line ${codeBlocks[i].num}. Extract to shared function.

✅ What to do:
1. Create utility function
2. Call from both places

Line ${codeBlocks[i].num}:
\`\`\`javascript
${codeBlocks[i].line}
\`\`\`

Line ${codeBlocks[j].num}:
\`\`\`javascript
${codeBlocks[j].line}
\`\`\``
          });
          this.violations.dryViolation++;
          break; // Only report first duplicate
        }
      }
    }
  }

  /**
   * Check 7: Nesting Depth > 3
   */
  checkNestingDepth(file, content) {
    const lines = content.split('\n');
    let maxDepth = 0;
    let maxDepthLine = 0;

    lines.forEach((line, idx) => {
      const depth = (line.match(/{/g) || []).length - (line.match(/}/g) || []).length;
      
      // Cumulative depth
      let currentDepth = 0;
      for (let i = 0; i <= idx; i++) {
        currentDepth += (lines[i].match(/{/g) || []).length;
        currentDepth -= (lines[i].match(/}/g) || []).length;
      }

      if (currentDepth > 3) {
        if (currentDepth > maxDepth) {
          maxDepth = currentDepth;
          maxDepthLine = idx + 1;
        }
      }
    });

    if (maxDepth > 3) {
      this.comments.push({
        path: file,
        line: maxDepthLine,
        side: 'RIGHT',
        body: `📍 **Nesting Too Deep** (Point 7)
Nesting depth is ${maxDepth} levels. Keep it ≤ 3.

✅ Refactoring strategies:
1. **Guard Clauses** (early return):
\`\`\`javascript
// Before (depth 5)
if (x) {
  if (y) {
    if (z) {
      doWork();
    }
  }
}

// After (depth 1)
if (!x) return;
if (!y) return;
if (!z) return;
doWork();
\`\`\`

2. **Extract Helper Functions**:
\`\`\`javascript
const isValidInput = (x, y, z) => x && y && z;
if (!isValidInput(x, y, z)) return;
doWork();
\`\`\``
      });
      this.violations.nestingDepth++;
    }
  }

  /**
   * Simple string similarity (Levenshtein-like)
   */
  stringSimilarity(a, b) {
    const longer = a.length > b.length ? a : b;
    const shorter = a.length > b.length ? b : a;
    
    if (longer.length === 0) return 1.0;
    
    const editDistance = this.levenshteinDistance(longer, shorter);
    return (longer.length - editDistance) / longer.length;
  }

  levenshteinDistance(a, b) {
    const matrix = [];

    for (let i = 0; i <= b.length; i++) {
      matrix[i] = [i];
    }

    for (let j = 0; j <= a.length; j++) {
      matrix[0][j] = j;
    }

    for (let i = 1; i <= b.length; i++) {
      for (let j = 1; j <= a.length; j++) {
        if (b.charAt(i - 1) === a.charAt(j - 1)) {
          matrix[i][j] = matrix[i - 1][j - 1];
        } else {
          matrix[i][j] = Math.min(
            matrix[i - 1][j - 1] + 1,
            matrix[i][j - 1] + 1,
            matrix[i - 1][j] + 1
          );
        }
      }
    }

    return matrix[b.length][a.length];
  }

  /**
   * Post all review comments to GitHub PR
   */
  async postReviewComments() {
    if (this.comments.length === 0) {
      console.log('✅ No violations found! Code is clean.');
      return;
    }

    const summary = `
## 🔍 AI Code Review Results

Found **${this.comments.length} issues** across 7-point hygiene checklist:

| Check | Count |
|-------|-------|
| 🧹 Dead Code | ${this.violations.deadCode} |
| 🧹 Unused Imports | ${this.violations.unusedImports} |
| ✨ Magic Numbers | ${this.violations.magicNumbers} |
| 📝 Bad Naming | ${this.violations.badNaming} |
| 🎯 SRP Violation | ${this.violations.srpViolation} |
| 🔄 DRY Violation | ${this.violations.dryViolation} |
| 📍 Nesting Depth | ${this.violations.nestingDepth} |

👉 **Fix these issues** and **re-request review** for approval.

---
`;

    // Post summary as PR comment
    await this.postPRComment(summary);

    // Post individual review comments
    for (const comment of this.comments) {
      await this.postReviewComment(comment);
    }

    console.log(`\n✅ Posted ${this.comments.length} review comments to PR #${this.prNumber}`);
  }

  /**
   * POST to GitHub API: Create PR comment
   */
  postPRComment(body) {
    return new Promise((resolve, reject) => {
      const data = JSON.stringify({ body });
      
      const options = {
        hostname: 'api.github.com',
        path: `/repos/${this.githubOwner}/${this.githubRepo}/issues/${this.prNumber}/comments`,
        method: 'POST',
        headers: {
          'Authorization': `token ${this.githubToken}`,
          'Accept': 'application/vnd.github.v3+json',
          'Content-Type': 'application/json',
          'Content-Length': Buffer.byteLength(data)
        }
      };

      const req = https.request(options, (res) => {
        if (res.statusCode === 201) {
          resolve();
        } else {
          reject(new Error(`Failed to post comment: ${res.statusCode}`));
        }
        res.on('data', () => {});
        res.on('end', () => resolve());
      });

      req.write(data);
      req.end();
    });
  }

  /**
   * POST to GitHub API: Create review comment on specific line
   */
  postReviewComment(comment) {
    return new Promise((resolve, reject) => {
      const data = JSON.stringify({
        body: comment.body,
        path: comment.path,
        line: comment.line,
        side: comment.side
      });

      const options = {
        hostname: 'api.github.com',
        path: `/repos/${this.githubOwner}/${this.githubRepo}/pulls/${this.prNumber}/comments`,
        method: 'POST',
        headers: {
          'Authorization': `token ${this.githubToken}`,
          'Accept': 'application/vnd.github.v3+json',
          'Content-Type': 'application/json',
          'Content-Length': Buffer.byteLength(data)
        }
      };

      const req = https.request(options, (res) => {
        res.on('data', () => {});
        res.on('end', () => {
          if (res.statusCode === 201) {
            resolve();
          } else {
            resolve(); // Don't fail, continue to next
          }
        });
      });

      req.on('error', () => resolve());
      req.write(data);
      req.end();
    });
  }

  /**
   * Main workflow
   */
  async run() {
    console.log(`\n🔍 AI Code Review started...`);
    console.log(`PR #${this.prNumber}`);
    console.log(`Files to review: ${this.files.length}\n`);

    for (const file of this.files) {
      if (!fs.existsSync(file)) {
        console.log(`⚠️  File not found: ${file}`);
        continue;
      }

      const content = fs.readFileSync(file, 'utf-8');
      console.log(`Checking ${file}...`);

      this.checkDeadCode(file, content);
      this.checkUnusedImports(file, content);
      this.checkMagicNumbers(file, content);
      this.checkBadNaming(file, content);
      this.checkSingleResponsibility(file, content);
      this.checkDRY(file, content);
      this.checkNestingDepth(file, content);
    }

    await this.postReviewComments();
  }
}

// CLI
if (require.main === module) {
  const args = process.argv.slice(2).reduce((acc, arg) => {
    const [key, value] = arg.split('=');
    acc[key.replace('--', '')] = value;
    return acc;
  }, {});

  const reviewer = new CodeReviewer(args);
  reviewer.run().catch(err => {
    console.error('❌ Error:', err);
    process.exit(1);
  });
}

module.exports = CodeReviewer;

Task 3: Demo — Create PR with Code Violations

Step 1: Create feature branch

bash
git checkout -b feature/user-validation

Step 2: Write code with violations

File: src/api.js (with intentional violations)

javascript
// ❌ Violation 1: Unused import
import axios from 'axios';
import moment from 'moment';  // unused

// ❌ Violation 2: Bad naming
const x = [];

// ❌ Violation 3: Magic numbers
const validateUser = (user) => {
  if (user.email.length > 255) throw new Error('Too long');
  if (user.password.length < 8) throw new Error('Too short');
  
  // ❌ Violation 4: SRP — does validation + logging + notification
  if (user.age > 18) {
    console.log('User age: ' + user.age);
    sendEmail(user.email);
  }
};

// ❌ Violation 5: Deep nesting
const processOrder = (order) => {
  if (order) {
    if (order.items) {
      if (order.items.length > 0) {
        if (order.customer) {
          if (order.customer.verified) {
            // Finally do work at depth 5
          }
        }
      }
    }
  }
};

Step 3: Commit & Push

bash
git add .
git commit -m "feat: add user validation"
git push origin feature/user-validation

Step 4: Create PR on GitHub

Go to GitHub → Create Pull Request

Step 5: Watch AI Review

  • GitHub Actions runs
  • AI Code Review workflow triggers
  • Posts violations as comments:
    • Line 2: "Unused import axios"
    • Line 3: "Unused import moment"
    • Line 5: "Bad variable name x"
    • Line 8: "Magic number 255"
    • Line 14: "SRP violation — does 3 things"
    • Line 25: "Nesting depth 5 — too deep"

Task 4: Respond with Evidence

Dev Response to Comment #1 (Unused Import):

I verified this: [evidence]
✅ Fixed. Removed axios import at line 2.
- axios was added for future plan to call external API
- But we're using native fetch() instead
- Removed unused import
- Added comment in ROADMAP.md about why fetch not axios

The suggestion is correct because unused imports increase bundle size and reduce readability.

Dev Response to Comment #5 (SRP):

I verified this: [evidence]
✅ Fixed. Split into 3 functions:
- isValidUser() — only validation (line 8)
- logValidation() — only logging (line 15)
- notifyNewUser() — only notification (line 19)

The suggestion is correct because:
1. Easier to test each function independently
2. Can reuse validation without logging
3. Can notify user without validation

Added unit tests: src/__tests__/api.test.js
- test/validation/valid-user → passes ✓
- test/validation/invalid-email → passes ✓
- test/notification/triggers-email → passes ✓

📝 Quiz (5 câu)

  1. Full Lifecycle: Nêu 3 phases của code review workflow (Part A, B, C)
  2. Anti-Pattern: Tại sao "Good catch! Fixed." là performative agreement? Cách đúng là gì?
  3. Response Framework: Có 4 loại situation. Khi nào dev nên "challenge with evidence"?
  4. 7-Point Checklist: Nêu 4 violations trong code và cách fix
  5. GitHub API: Comment review dùng endpoint nào? (path, method)

🏠 Homework (30 phút)

  1. Create PR: Commit code với 3-4 violations
  2. Watch Review: AI posts comments
  3. Respond: Fix violations + respond with evidence (not just "fixed")
  4. Re-request: Re-request review
  5. Approve: Merge when approved

📚 Resources

Powered by CodyMaster × VitePress