157 lines
7.1 KiB
Markdown
157 lines
7.1 KiB
Markdown
# 🔍 NexusReader Code Review Backlog
|
|
|
|
## 🔴 CRITICAL — Fix Before Next Release
|
|
|
|
- **Status:** ✅ Resolved (2026-05-03)
|
|
- **Implementation:** Removed `AddMediatR` from `AddApplication()` and `AddInfrastructure()`. Unified registration in Host (`Program.cs`, `MauiProgram.cs`). Added `IInfrastructureMarker` and a startup validation check in `Web.New` that throws `InvalidOperationException` if `AddInfrastructure` is missing.
|
|
- **DoD:** Application fails fast with a clear error if `AddInfrastructure()` is omitted.
|
|
|
|
---
|
|
|
|
- **Status:** ✅ Resolved (2026-05-03)
|
|
- **Implementation:** Added `VerifyGroundednessAsync` to `IKnowledgeService` and implemented it in `KnowledgeService` (Infrastructure). Updated `VerifyGroundednessCommandHandler` in Application to inject `IKnowledgeService` instead of `IChatClient`.
|
|
- **DoD:** No `IChatClient` or `IEmbeddingGenerator` references remain in `NexusReader.Application`.
|
|
|
|
---
|
|
|
|
- **Status:** ✅ Resolved (2026-05-03)
|
|
- **Implementation:** Threaded `tenantId` through all `IKnowledgeService` methods and `ProcessKnowledgeUnitsAsync`. Scoped `SemanticKnowledgeCache` and `KnowledgeUnit` lookups/writes to the provided `tenantId`. Updated API endpoints in `Program.cs` and `WasmKnowledgeService` to pass the authenticated user's `TenantId`.
|
|
- **DoD:** No hardcoded `"global"` TenantId in write paths. Extracted units are always scoped to the caller's tenant.
|
|
|
|
---
|
|
|
|
- **Status:** ✅ Resolved (2026-05-03)
|
|
- **Implementation:** Changed `NexusUser.TenantId` from `Guid` to `string`. All entities now use `string` for `TenantId`, allowing the use of `"global"` as a sentinel value.
|
|
- **DoD:** All entities use the same `TenantId` type. All query filters are consistent.
|
|
|
|
---
|
|
|
|
## 🟠 MAJOR — High Priority Fixes
|
|
|
|
### [MJ-01] Missing Exception Handling in `EpubService`
|
|
|
|
- **File:** `Infrastructure/Services/EpubService.cs:45`
|
|
- **Problem:** The service uses raw `ZipArchive` operations without try-catch blocks. Corrupt EPUB files will crash the circuit instead of returning a `Result.Fail`.
|
|
- **Action:** Wrap the extraction logic in a try-catch and return `Result.Fail<EpubContent>(ex.Message)`.
|
|
- **DoD:** Uploading a renamed `.txt` as `.epub` returns a user-friendly error instead of a 500 error.
|
|
|
|
---
|
|
|
|
### [MJ-02] Hardcoded Pricing & Limits in Stripe Logic
|
|
|
|
- **File:** `Web.New/Program.cs:298`
|
|
- **Problem:** Subscription limits (50k tokens for Pro) are hardcoded in the webhook handler. Changing prices or limits requires a code redeploy.
|
|
- **Action:** Move limits to `appsettings.json` or a `SubscriptionPlan` domain entity. Use `IOptions<SubscriptionSettings>` in the handler.
|
|
- **DoD:** Limits can be changed via configuration without rebuilding the app.
|
|
|
|
---
|
|
|
|
### [MJ-03] Knowledge Graph: Circular Dependency Potential
|
|
|
|
- **File:** `UI.Shared/Services/KnowledgeGraphService.cs`
|
|
- **Problem:** The service manages its own state but is injected as `Scoped`. If multiple components use it, they share the same graph state, which might lead to race conditions during navigation.
|
|
- **Action:** Ensure the service is either stateless (returning data) or implement a `Clear()` method called on `OnInitialized`.
|
|
- **DoD:** Navigating between two different books correctly clears the graph.
|
|
|
|
---
|
|
|
|
### [MJ-04] Insecure `Profile` Endpoint Exposes Internal IDs
|
|
|
|
- **File:** `Web.New/Program.cs:366`
|
|
- **Problem:** The `/identity/profile` endpoint returns the raw `TenantId` and internal database IDs in the JSON response.
|
|
- **Action:** Create a `UserProfileDto` and use Mapster to exclude internal metadata.
|
|
- **DoD:** Sensitive internal GUIDs/IDs are not visible in the browser's Network tab.
|
|
|
|
---
|
|
|
|
### [MJ-05] Missing Database Index for Multi-Tenancy
|
|
|
|
- **Problem:** `TenantId` is used in almost every query (KnowledgeUnits, Cache, Users) but lacks a database index. As data grows, retrieval will slow down significantly (O(N) vs O(log N)).
|
|
- **Action:** Add `HasIndex(x => x.TenantId)` to the `AppDbContext` configuration for all relevant entities.
|
|
- **DoD:** EF Migration generated with `CREATE INDEX` for `TenantId`.
|
|
|
|
---
|
|
|
|
### [MJ-06] KM-RAG: Link Integrity is Not Validated
|
|
|
|
- **File:** `Infrastructure/Services/KnowledgeService.cs:208`
|
|
- **Problem:** When processing `KnowledgeUnitLink`, the service assumes both `Source` and `Target` units exist in the DB. If AI returns a link to a non-existent node, the DB insert will fail (foreign key violation).
|
|
- **Action:** Add a check to verify both units exist or are being created in the same batch before adding the link.
|
|
- **DoD:** Broken links from AI are logged as warnings and skipped, not causing a total failure.
|
|
|
|
---
|
|
|
|
### [MJ-07] Ebook Entity Missing Tenant Isolation
|
|
|
|
- **File:** `Domain/Entities/Ebook.cs`
|
|
- **Problem:** The `Ebook` entity lacks a `TenantId` property. All uploaded books are visible to all users if the ID is guessed.
|
|
- **Action:** Add `TenantId` to `Ebook` and filter all queries in `EpubService`.
|
|
- **DoD:** User A cannot see User B's books.
|
|
|
|
---
|
|
|
|
### [MJ-08] QuizResults Missing Tenant Isolation
|
|
|
|
- **File:** `Domain/Entities/QuizResult.cs`
|
|
- **Problem:** Similar to ebooks, quiz results are not scoped to a tenant.
|
|
- **Action:** Add `TenantId` to `QuizResult`.
|
|
- **DoD:** Results are correctly partitioned.
|
|
|
|
---
|
|
|
|
## 🟡 MINOR — Technical Debt & UX
|
|
|
|
### [MN-01] Missing Logging in `KnowledgeCoordinator`
|
|
- **Action:** Add `ILogger<KnowledgeCoordinator>` and log successful/failed extraction steps.
|
|
|
|
### [MN-02] Hardcoded "Gemini-1.5-Flash" in Domain
|
|
- **File:** `Domain/Entities/SemanticKnowledgeCache.cs:20`
|
|
- **Action:** Move the default model ID to a constant in `AiSettings`.
|
|
|
|
### [MN-03] UI: Shimmer Effect Lack Animation
|
|
- **File:** `UI.Shared/Components/Molecules/GroundednessBadge.razor`
|
|
- **Action:** Add `@keyframes` for the shimmer effect in CSS.
|
|
|
|
### [MN-04] Identity: Google Callback Lack Error Handling
|
|
- **File:** `Web.New/Program.cs:340`
|
|
- **Action:** Better UI feedback when `ExternalLoginInfo` is null.
|
|
|
|
### [MN-05] Tokenizer Initialization is Expensive
|
|
- **File:** `Infrastructure/Services/KnowledgeService.cs:43`
|
|
- **Action:** Make `_tokenizer` static or Singleton to avoid recreating it per request.
|
|
|
|
### [MN-06] Mapster: Global Configuration Check
|
|
- **Action:** Ensure `TypeAdapterConfig.GlobalSettings.Scan(...)` is only called once.
|
|
|
|
### [MN-07] SignalR: Missing Reconnection Logic
|
|
- **Action:** Implement `hubConnection.OnReconnected` in `SyncService.cs`.
|
|
|
|
### [MN-08] CSS: Z-Index Consistency
|
|
- **Action:** Define a `z-index` scale in `index.css`.
|
|
|
|
### [MN-09] SEO: Missing Meta Descriptions
|
|
- **Action:** Update `App.razor` with dynamic meta tags.
|
|
|
|
### [MN-10] Performance: Large EPUB Parsing
|
|
- **Action:** Implement streaming extraction for EPUBs over 10MB.
|
|
|
|
---
|
|
|
|
## 🧪 TESTING — Coverage Gaps
|
|
|
|
### [TEST-01] Integration Tests for KM-RAG Retrieval
|
|
- **Action:** Create `tests/NexusReader.IntegrationTests`.
|
|
- **Scenario:** Ingest a document, then verify that `GetRelevantContext` returns the correct snippets with tenant isolation active.
|
|
|
|
---
|
|
|
|
## 📊 Summary Table
|
|
|
|
| Severity | Count | Status |
|
|
|---|---|---|
|
|
| 🔴 Critical | 4 | 4 resolved |
|
|
| 🟠 Major | 8 | Unresolved |
|
|
| 🟡 Minor | 10 | Unresolved |
|
|
| 🧪 Tests | 1 | Unresolved |
|
|
| **Total** | **23** | **4 resolved** |
|