|
| 1 | +--- |
| 2 | +name: code-review |
| 3 | +description: Automated PR review using comprehensive checklist tailored for Contentstack CLI plugins |
| 4 | +--- |
| 5 | + |
| 6 | +# Code Review Command |
| 7 | + |
| 8 | +## Usage Patterns |
| 9 | + |
| 10 | +### Scope-Based Reviews |
| 11 | +- `/code-review` - Review all current changes with full checklist |
| 12 | +- `/code-review --scope typescript` - Focus on TypeScript configuration and patterns |
| 13 | +- `/code-review --scope testing` - Focus on Mocha/Chai/Sinon test patterns |
| 14 | +- `/code-review --scope contentstack` - Focus on API integration and CLI patterns |
| 15 | +- `/code-review --scope oclif` - Focus on command structure and OCLIF patterns |
| 16 | + |
| 17 | +### Severity Filtering |
| 18 | +- `/code-review --severity critical` - Show only critical issues (security, breaking changes) |
| 19 | +- `/code-review --severity high` - Show high and critical issues |
| 20 | +- `/code-review --severity all` - Show all issues including suggestions |
| 21 | + |
| 22 | +### Package-Aware Reviews |
| 23 | +- `/code-review --package contentstack-import` - Review changes in specific package |
| 24 | +- `/code-review --package-type plugin` - Review plugin packages only |
| 25 | +- `/code-review --package-type library` - Review library packages (e.g., variants) |
| 26 | + |
| 27 | +### File Type Focus |
| 28 | +- `/code-review --files commands` - Review command files only |
| 29 | +- `/code-review --files tests` - Review test files only |
| 30 | +- `/code-review --files modules` - Review import/export modules |
| 31 | + |
| 32 | +## Comprehensive Review Checklist |
| 33 | + |
| 34 | +### Monorepo Structure Compliance |
| 35 | +- **Package organization**: Proper placement in `packages/` structure |
| 36 | +- **pnpm workspace**: Correct `package.json` workspace configuration |
| 37 | +- **Build artifacts**: No `lib/` directories committed to version control |
| 38 | +- **Dependencies**: Proper use of shared utilities (`@contentstack/cli-command`, `@contentstack/cli-utilities`) |
| 39 | + |
| 40 | +### TypeScript Standards (Repository-Specific) |
| 41 | +- **Configuration compliance**: Follows package-specific TypeScript config |
| 42 | +- **Naming conventions**: kebab-case files, PascalCase classes, camelCase functions |
| 43 | +- **Type safety**: Appropriate use of strict mode vs relaxed settings per package |
| 44 | +- **Import patterns**: ES modules with proper default/named exports |
| 45 | +- **Migration strategy**: Proper use of `@ts-ignore` during gradual migration |
| 46 | + |
| 47 | +### OCLIF Command Patterns (Actual Implementation) |
| 48 | +- **Base class usage**: Extends `@contentstack/cli-command` (not `@oclif/core`) |
| 49 | +- **Command structure**: Proper `static description`, `examples`, `flags` |
| 50 | +- **Topic organization**: Uses `cm` topic structure (`cm:stacks:import`) |
| 51 | +- **Error handling**: Uses `handleAndLogError` from utilities |
| 52 | +- **Validation**: Early flag validation and user-friendly error messages |
| 53 | +- **Service delegation**: Commands orchestrate, services implement business logic |
| 54 | + |
| 55 | +### Testing Excellence (Mocha/Chai/Sinon Stack) |
| 56 | +- **Framework compliance**: Uses Mocha + Chai + Sinon (not Jest) |
| 57 | +- **File patterns**: Follows `*.test.ts` naming (or `*.test.js` for bootstrap) |
| 58 | +- **Directory structure**: Proper placement in `test/unit/`, `test/lib/`, etc. |
| 59 | +- **Mock patterns**: Proper Sinon stubbing of SDK methods |
| 60 | +- **Coverage configuration**: Correct nyc setup (watch for `inlcude` typo) |
| 61 | +- **Test isolation**: Proper `beforeEach`/`afterEach` with `sinon.restore()` |
| 62 | +- **No real API calls**: All external dependencies properly mocked |
| 63 | + |
| 64 | +### Contentstack API Integration (Real Patterns) |
| 65 | +- **SDK usage**: Proper `managementSDKClient` and fluent API chaining |
| 66 | +- **Authentication**: Correct `configHandler` and token alias handling |
| 67 | +- **Rate limiting compliance**: |
| 68 | + - Batch spacing (minimum 1 second between batches) |
| 69 | + - 429 retry handling with exponential backoff |
| 70 | + - Pagination throttling for variants |
| 71 | +- **Error handling**: Proper `handleAndLogError` usage and user-friendly messages |
| 72 | +- **Configuration**: Proper regional endpoint and management token handling |
| 73 | + |
| 74 | +### Import/Export Module Architecture |
| 75 | +- **BaseClass extension**: Proper inheritance from import/export BaseClass |
| 76 | +- **Batch processing**: Correct use of `makeConcurrentCall` and `logMsgAndWaitIfRequired` |
| 77 | +- **Module organization**: Proper entity-specific module structure |
| 78 | +- **Configuration handling**: Proper `ModuleClassParams` usage |
| 79 | +- **Progress feedback**: Appropriate user feedback during long operations |
| 80 | + |
| 81 | +### Security and Best Practices |
| 82 | +- **Token security**: No API keys or tokens logged or committed |
| 83 | +- **Input validation**: Proper validation of user inputs and flags |
| 84 | +- **Error exposure**: No sensitive information in error messages |
| 85 | +- **File permissions**: Proper handling of file system operations |
| 86 | +- **Process management**: Appropriate use of `process.exit(1)` for critical failures |
| 87 | + |
| 88 | +### Performance Considerations |
| 89 | +- **Concurrent processing**: Proper use of `Promise.allSettled` for batch operations |
| 90 | +- **Memory management**: Appropriate handling of large datasets |
| 91 | +- **Rate limiting**: Compliance with Contentstack API limits (10 req/sec) |
| 92 | +- **Batch sizing**: Appropriate batch sizes for different operations |
| 93 | +- **Progress tracking**: Efficient progress reporting without performance impact |
| 94 | + |
| 95 | +### Package-Specific Patterns |
| 96 | +- **Plugin vs Library**: Correct `oclif.commands` configuration for plugin packages |
| 97 | +- **Command compilation**: Proper build pipeline (`tsc` → `lib/commands` → `oclif manifest`) |
| 98 | +- **Dependency management**: Correct use of shared vs package-specific dependencies |
| 99 | +- **Test variations**: Handles different test patterns per package (JS vs TS, different structures) |
| 100 | + |
| 101 | +## Review Execution |
| 102 | + |
| 103 | +### Automated Checks |
| 104 | +1. **Lint compliance**: ESLint and TypeScript compiler checks |
| 105 | +2. **Test coverage**: nyc coverage thresholds (where enforced) |
| 106 | +3. **Build verification**: Successful compilation to `lib/` directories |
| 107 | +4. **Dependency audit**: No security vulnerabilities in dependencies |
| 108 | + |
| 109 | +### Manual Review Focus Areas |
| 110 | +1. **API integration patterns**: Verify proper SDK usage and error handling |
| 111 | +2. **Rate limiting implementation**: Check for proper throttling mechanisms |
| 112 | +3. **Test quality**: Verify comprehensive mocking and error scenario coverage |
| 113 | +4. **Command usability**: Ensure clear help text and examples |
| 114 | +5. **Monorepo consistency**: Check for consistent patterns across packages |
| 115 | + |
| 116 | +### Common Issues to Flag |
| 117 | +- **Coverage config typos**: `"inlcude"` instead of `"include"` in `.nycrc.json` |
| 118 | +- **Inconsistent TypeScript**: Mixed strict mode usage without migration plan |
| 119 | +- **Real API calls in tests**: Any unmocked external dependencies |
| 120 | +- **Missing rate limiting**: API calls without proper throttling |
| 121 | +- **Build artifacts committed**: Any `lib/` directories in version control |
| 122 | +- **Inconsistent error handling**: Not using utilities error handling patterns |
0 commit comments