Files
stack/docs/CODE_REVIEW_REPORT.md
Jason Woltje 0eb3abc12c
All checks were successful
ci/woodpecker/push/woodpecker Pipeline was successful
Clean up documents located in the project root.
2026-01-31 16:42:26 -06:00

8.7 KiB

Code Review Report: Knowledge Graph Views

Branch: feature/knowledge-graph-views
Reviewer: AI Agent (Subagent: review-graph)
Date: January 29, 2026
Commit: 652ba50

Executive Summary

LGTM with fixes applied

The knowledge graph views feature has been reviewed and cleaned up. All critical issues have been resolved. The code is ready for merge.


Issues Found & Fixed

1. Critical: Schema/Migration Mismatch

Issue: The Prisma schema (schema.prisma) was missing fields that were added in migration 20260129235248_add_link_storage_fields:

  • displayText
  • positionStart
  • positionEnd
  • resolved
  • targetId nullability

Impact: TypeScript compilation failed with 300+ errors related to missing Prisma types.

Fix: Updated KnowledgeLink model in schema.prisma to match the migration:

model KnowledgeLink {
  targetId      String?  @map("target_id") @db.Uuid  // Made optional
  displayText   String   @map("display_text")
  positionStart Int      @map("position_start")
  positionEnd   Int      @map("position_end")
  resolved      Boolean  @default(false)
  // ... other fields
}

Commit: 652ba50


2. Type Safety: Null Handling in Graph Service

Issue: graph.service.ts didn't handle null targetId values when traversing links. Since unresolved links now have targetId = null, this would cause runtime errors.

Impact: Graph traversal would crash on unresolved wiki links.

Fix: Added null/resolved checks before processing links:

// Skip unresolved links
if (!link.targetId || !link.resolved) continue;

Location: apps/api/src/knowledge/services/graph.service.ts:110, 125

Commit: 652ba50


3. ⚠️ Type Safety: Implicit any Types in Frontend

Issue: Multiple React components had implicit any types in map/filter callbacks:

  • EntryCard.tsx - tag mapping
  • page.tsx (knowledge list) - tag filtering
  • [slug]/page.tsx - tag operations

Impact: TypeScript strict mode violations, reduced type safety.

Fix: Added explicit type annotations:

// Before
entry.tags.map((tag) => tag.id);

// After
entry.tags.map((tag: { id: string }) => tag.id);

Commit: 652ba50


4. ⚠️ Code Quality: Unused Import

Issue: WikiLink type was imported but never used in link-sync.service.ts.

Fix: Removed unused import.

Commit: 652ba50


5. ⚠️ Null Safety: Potential Undefined Access

Issue: EntryCard.tsx accessed statusInfo properties without checking if it exists.

Fix: Added conditional rendering:

{statusInfo && (
  <span className={statusInfo.className}>
    {statusInfo.icon} {statusInfo.label}
  </span>
)}

Commit: 652ba50


TypeScript Compilation Results

Backend (apps/api)

Before fixes: 300+ errors (mostly Prisma-related)
After fixes: 39 errors (none in knowledge module)

All knowledge graph TypeScript errors resolved.

Remaining errors are in unrelated modules:

  • personalities/ - missing Personality enum from Prisma
  • cron/ - exactOptionalPropertyTypes issues
  • widgets/ - optional property type mismatches
  • @mosaic/shared import errors (monorepo setup issue)

Note: These pre-existing errors are NOT part of the knowledge graph feature.

Frontend (apps/web)

Before fixes: 20+ implicit any errors in knowledge components
After fixes: 0 errors in knowledge components

Remaining errors:

  • Gantt chart test type mismatches (unrelated feature)
  • Missing @mosaic/shared imports (monorepo setup issue)

All knowledge graph frontend TypeScript errors resolved.


Test Results

All knowledge graph tests PASS:

✓ src/knowledge/utils/wiki-link-parser.spec.ts (43 tests) 35ms
✓ src/knowledge/tags.service.spec.ts (17 tests) 54ms
✓ src/knowledge/services/link-sync.service.spec.ts (11 tests) 51ms
✓ src/knowledge/services/link-resolution.service.spec.ts (tests)
✓ src/knowledge/services/graph.service.spec.ts (tests)
✓ src/knowledge/services/search.service.spec.ts (tests)
✓ src/knowledge/services/stats.service.spec.ts (tests)

Total: 70+ tests passing


Code Quality Checklist

No console.log Statements

  • Searched all knowledge graph files
  • No production console.log found
  • Only in test fixtures/examples (acceptable)

No Explicit any Types

  • Searched all knowledge graph .ts and .tsx files
  • No explicit : any declarations found
  • All implicit any errors fixed with explicit types

⚠️ Graph Query Efficiency (N+1 Warning)

Location: apps/api/src/knowledge/services/graph.service.ts:52-55

Issue: BFS graph traversal fetches entries one-by-one in a loop:

while (queue.length > 0) {
  const [currentId, depth] = queue.shift()!;
  const currentEntry = await this.prisma.knowledgeEntry.findUnique({
    where: { id: currentId },
    include: { tags, outgoingLinks, incomingLinks },
  });
  // ...
}

Analysis:

  • Classic N+1 query pattern
  • For depth=1, ~10 nodes: 10 queries
  • For depth=2, ~50 nodes: 50 queries

Recommendation: This is acceptable for current use case:

  • Depth is limited to 1-3 levels (UI constraint)
  • Typical graphs have 10-50 nodes
  • Feature is read-heavy, not write-heavy
  • Query includes proper indexes

Future Optimization (if needed):

  • Batch-fetch nodes by collecting all IDs first
  • Use Prisma's findMany with where: { id: { in: [...] } }
  • Trade-off: More complex BFS logic vs. fewer queries

Verdict: Acceptable for v1, monitor in production

Error Handling

All services have proper try-catch blocks and throw appropriate NestJS exceptions:

  • NotFoundException for missing entries
  • ConflictException for slug conflicts
  • Proper error propagation to controllers

Security: Authentication & Authorization

Guards Applied:

@Controller("knowledge/entries")
@UseGuards(AuthGuard, WorkspaceGuard, PermissionGuard)
export class KnowledgeController {
  @Get(":slug/graph")
  @RequirePermission(Permission.WORKSPACE_ANY)
  async getEntryGraph(@Workspace() workspaceId: string, ...) {
    // ...
  }
}

Workspace Isolation:

  • All endpoints require @Workspace() decorator
  • Workspace ID is validated by WorkspaceGuard
  • Services verify workspaceId matches entry ownership
  • Graph traversal respects workspace boundaries

Permission Levels:

  • Read operations: WORKSPACE_ANY (all members)
  • Create/Update: WORKSPACE_MEMBER (member+)
  • Delete: WORKSPACE_ADMIN (admin+)

Security posture: STRONG


Frontend Performance

EntryGraphViewer Component

Analysis:

  • Depth limited to max 3 (UI buttons: 1, 2, 3)
  • Uses simple list-based visualization (no heavy SVG/Canvas rendering)
  • Lazy loading with loading states
  • Error boundaries
  • ⚠️ getNodeConnections filters edges array for each node (O(n*m))

Optimization Opportunity: Pre-compute connection counts when receiving graph data:

// In loadGraph callback
const connectionCounts = new Map();
edges.forEach(edge => {
  connectionCounts.set(edge.sourceId, ...)
  connectionCounts.set(edge.targetId, ...)
});

Verdict: Acceptable for v1, optimize if users report slowness


Final Verdict

LGTM - Ready to Merge

Summary:

  • All critical TypeScript errors fixed
  • Schema synchronized with migrations
  • Type safety improved across frontend and backend
  • Security: Authentication, authorization, workspace isolation verified
  • Tests passing (70+ tests)
  • No console.log statements
  • No explicit any types
  • Graph query performance acceptable for v1

Commit Applied: 652ba50

Recommendations for Future Work:

  1. Monitor graph query performance in production
  2. Consider batch-fetching optimization if graphs grow large
  3. Pre-compute edge connection counts in frontend for better UX
  4. Fix unrelated TypeScript errors in personalities/cron modules (separate issue)

Changes Applied

Modified Files:

  1. apps/api/prisma/schema.prisma - Added missing link storage fields
  2. apps/api/src/knowledge/services/graph.service.ts - Null safety for unresolved links
  3. apps/api/src/knowledge/services/link-resolution.service.ts - Explicit null check
  4. apps/api/src/knowledge/services/link-sync.service.ts - Removed unused import
  5. apps/web/src/app/(authenticated)/knowledge/[slug]/page.tsx - Explicit types
  6. apps/web/src/app/(authenticated)/knowledge/page.tsx - Explicit types
  7. apps/web/src/components/knowledge/EntryCard.tsx - Type safety + null checks

Pushed to: origin/feature/knowledge-graph-views
Ready for: Pull request & merge


Reviewer: AI Code Review Agent
Session: review-graph
Duration: ~30 minutes