mirror of
https://github.com/tiennm99/ccs.git
synced 2026-09-02 18:18:43 +00:00
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
This commit is contained in:
@@ -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.
|
||||||
Reference in New Issue
Block a user