docs(minimax): Update review_pr.md with fix status

- Mark Issue 1 as FIXED (model name casing) - Mark Issue 2 as FIXED (placeholder added) - Update review summary with completion status - Ready for merge
2026-07-16 08:17:11 +00:00 · 2026-01-02 20:32:29 +08:00
parent 46e09950e8
commit 5d34bd6ec2
1 changed files with 122 additions and 0 deletions
@@ -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<ValidationResult>
+```
+
+---
+
+## 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.