diff --git a/docs/review_pr.md b/docs/review_pr.md new file mode 100644 index 00000000..42d26287 --- /dev/null +++ b/docs/review_pr.md @@ -0,0 +1,122 @@ +# Code Review: PR #250 - MiniMax M2.1 Support + +**PR:** https://github.com/kaitranntt/ccs/pull/250 +**Author:** jellydn (Dung Duc Huynh) +**Date:** 2026-01-02 +**Status:** Changes Requested + +--- + +## Overview + +The PR adds full MiniMax M2.1 support to CCS, following the GLM implementation pattern. Changes include: +- Settings template: `config/base-minimax.settings.json` +- Model pricing: `src/web-server/model-pricing.ts` +- API key validation: `src/utils/api-key-validator.ts` +- Integration: `src/ccs.ts` + +--- + +## Issues Found + +### 1. ✅ FIXED: Model Name Case Mismatch + +**Files:** `config/base-minimax.settings.json:8`, `src/web-server/model-pricing.ts:549` + +**Problem:** The model name was inconsistent between files: +- Settings: `MiniMax-M2.1-Lightning` (capital "L") +- Pricing: `MiniMax-M2.1-lightning` (lowercase "l") + +**Impact:** Dashboard Analytics would fail to find pricing for the Haiku tier model, causing incorrect cost calculations. + +**Fix:** Changed `MiniMax-M2.1-Lightning` to `MiniMax-M2.1-lightning` in settings file. + +--- + +### 2. ✅ FIXED: Missing MiniMax Placeholder + +**File:** `src/utils/api-key-validator.ts:19-26` + +**Problem:** `DEFAULT_PLACEHOLDERS` did not include `YOUR_MINIMAX_API_KEY_HERE` even though: +- It's the placeholder used in `base-minimax.settings.json` +- Other providers have their specific placeholders + +**Current behavior:** The generic `YOUR_API_KEY_HERE` catches it, but inconsistent. + +**Fix:** Added `YOUR_MINIMAX_API_KEY_HERE` to `DEFAULT_PLACEHOLDERS` array. + +--- + +### 3. Code Duplication with GLM Validator (LOW - Technical Debt) + +**File:** `src/utils/api-key-validator.ts` + +**Problem:** `validateMiniMaxKey` (~95 lines) is nearly identical to `validateGlmKey` (~95 lines). Only differences: +- Default base URL +- Error message content +- Suggestion message content + +**Impact:** Future maintenance requires updating both functions. Risk of divergence. + +**Future Fix (optional):** Extract a common helper function: +```typescript +async function validateApiKeyCommon( + apiKey: string, + providerName: string, + defaultBaseUrl: string, + errorMessages: { rejected: string; suggestion: string } +): Promise +``` + +--- + +## What's Working Well + +| Aspect | Status | +|--------|--------| +| Follows GLM pattern | OK | +| Fail-open design on network errors | OK | +| Settings template format | OK | +| Model pricing entries | OK | +| Provider presets sync (UI + API) | OK | +| Placeholder detection | OK | +| Skip pre-flight via env var | OK | + +--- + +## Required Changes (Before Merge) + +1. **Fix Issue 1:** Change `MiniMax-M2.1-Lightning` to `MiniMax-M2.1-lightning` in settings +2. **Fix Issue 2:** Add `YOUR_MINIMAX_API_KEY_HERE` to `DEFAULT_PLACEHOLDERS` + +--- + +## Optional Changes (Technical Debt) + +3. Consider refactoring validators to share common code (Issue 3) + +--- + +## Verification Commands + +```bash +# After fixes, run validation +bun run validate + +# Check settings file +cat config/base-minimax.settings.json + +# Verify pricing lookup works (manually test if needed) +``` + +--- + +## Review Summary + + | Issue | Severity | Type | Status | +|-------|----------|------|--------| +| Model name case mismatch | Medium | Bug | ✅ FIXED | +| Missing MiniMax placeholder | Low | Inconsistency | ✅ FIXED | +| Code duplication with GLM | Low | Tech debt | Optional | + +**Status:** All required issues (1 & 2) resolved. Ready for merge.