3fbbb1a93b
- Next.js 14+ with App Router and TypeScript - Tailwind CSS and ShadCN UI styling - Zustand state management - Dexie.js for IndexedDB (local-first data) - Auth.js v5 for authentication - BMAD framework integration Co-Authored-By: Claude <noreply@anthropic.com>
25 lines
2.0 KiB
Markdown
25 lines
2.0 KiB
Markdown
**🔥 CODE REVIEW FINDINGS, Max!**
|
|
|
|
**Story:** 4-3-model-selection-configuration.md
|
|
**Git vs Story Discrepancies:** 0 found (Repo has no commits, verified file content)
|
|
**Issues Found:** 0 High, 2 Medium, 4 Low
|
|
|
|
## 🔴 CRITICAL ISSUES
|
|
*None. Acceptance Criteria are met.*
|
|
|
|
## 🟡 MEDIUM ISSUES
|
|
1. **UX Data Loss in ProviderForm**: Selecting a provider preset (OpenAI/DeepSeek/etc.) immediately overwrites the "Model Name" field. If a user enters a custom model first and then clicks a preset (e.g., to set the URL), their custom model name is lost of the user has not saved it yet.
|
|
* *File:* `src/components/features/settings/provider-form.tsx`
|
|
2. **Weak Input Validation**: The "Model Name" field accepts *any* non-empty string. There is no pattern validation (e.g., preventing whitespace-only strings if trimmed check fails differently, or weird chars) or reasonable length limits.
|
|
* *File:* `src/services/settings-service.ts`
|
|
|
|
## 🟢 LOW ISSUES
|
|
1. **Weak Security**: API Keys are "encrypted" using `btoa()` (Base64). This is trivial to decode and provides only visual obfuscation, not real security.
|
|
* *File:* `src/store/use-settings.ts`
|
|
2. **Connection Validation Robustness**: `LLMService.validateConnection` requests `max_tokens: 1` with message "hello". Some models or provider safety filters might reject this pattern, leading to false negatives in validation.
|
|
* *File:* `src/services/llm-service.ts`
|
|
3. **Documentation Gap**: The story's "Files Modified" list misses `src/services/settings-service.ts` (where validation logic lives) and `src/services/provider-management-service.ts` (used by ChatService), which are critical for the integration verification.
|
|
* *File:* `_bmad-output/implementation-artifacts/4-3-model-selection-configuration.md`
|
|
4. **Test Fragility**: `resetStore()` in tests clears *all* `localStorage`. While fine now, this is aggressive and makes tests brittle if we add other persistent features later.
|
|
* *File:* `src/components/features/settings/provider-form.model-selection.test.tsx`
|