diff --git a/docs/pr-reviews/PR-014-major-update.md b/docs/pr-reviews/PR-014-major-update.md new file mode 100644 index 000000000..6bb024b2d --- /dev/null +++ b/docs/pr-reviews/PR-014-major-update.md @@ -0,0 +1,272 @@ +# Code Review: PR #14 + +**Title:** feat: major update with network graphs, themes, todo extensions, and more +**Author:** Till-JS +**Date:** 2025-12-10 +**Status:** OPEN +**URL:** https://github.com/Memo-2023/manacore-monorepo/pull/14 + +--- + +## Summary + +| Metric | Value | +|--------|-------| +| Files Changed | 382 | +| Additions | +39,514 | +| Deletions | -6,251 | + +--- + +## Overview + +This is a **major feature release** introducing: + +1. **Network Graph Visualization** - D3.js force-directed graphs for Contacts, Calendar, and Todo apps +2. **Central Tags API** - Unified tagging system in mana-core-auth +3. **Custom Themes System** - Theme editor, community gallery, and sharing +4. **Todo App Extensions** - Kanban boards, statistics, settings page, PWA support +5. **Contacts App Features** - Duplicate detection, photo upload, batch operations, favorites views +6. **Help System** - Shared packages for content, UI, and types (`shared-help-content`, `shared-help-ui`, `shared-help-types`, `shared-help-mobile`) +7. **Skeleton Loaders** - Better loading states across apps +8. **CommandBar** - Global search (Cmd+K) +9. **Bug Fixes** - Network graph simulation fixes, database schema TEXT for user_id + +--- + +## Code Quality Analysis + +### Strengths + +#### 1. Excellent Architecture +- Clean separation of concerns with shared packages (`shared-ui`, `shared-theme`, `shared-tags`, `shared-help-*`) +- Proper Svelte 5 runes usage (`$state`, `$derived`, `$effect`) +- Good TypeScript typing throughout + +#### 2. NetworkGraph Component (`packages/shared-ui/src/organisms/network/`) +- Well-structured D3.js integration with `d3-zoom` and `d3-selection` +- Proper zoom/pan handling +- Keyboard shortcuts implemented: + - `+`/`-` for zoom in/out + - `0` to reset zoom + - `Esc` to deselect + - `F` to focus on selected node + - `/` to focus search +- Accessible with `role="button"`, `aria-label`, `tabindex` +- Efficient re-rendering with proper state management + +#### 3. Tags Service (`services/mana-core-auth/src/tags/`) +- Proper validation (duplicate name check before create/update) +- Good use of Drizzle's `returning()` for immediate results +- User-scoped queries with proper authorization (`userId` checks) +- Default tags created for new users + +#### 4. Custom Themes Store (`packages/shared-theme/src/custom-themes-store.svelte.ts`) +- Clean API design with factory function pattern +- Proper state management with Svelte 5 runes +- Good separation of public/authenticated API calls +- CSS variable application for runtime theming + +--- + +### Suggestions for Improvement + +#### 1. Hardcoded German Strings + +**Location:** `packages/shared-ui/src/organisms/network/NetworkGraph.svelte:440-442` + +```svelte +

Keine Verbindungen gefunden

+

Elemente werden verbunden, wenn sie gemeinsame Tags haben.

+``` + +**Recommendation:** Use i18n for user-facing strings to maintain consistency across the monorepo. + +--- + +#### 2. Default Tags in German Only + +**Location:** `services/mana-core-auth/src/tags/tags.service.ts:10-15` + +```typescript +const DEFAULT_TAGS = [ + { name: 'Arbeit', color: '#3B82F6', icon: 'Briefcase' }, + { name: 'Persönlich', color: '#10B981', icon: 'User' }, + { name: 'Familie', color: '#EC4899', icon: 'Heart' }, + { name: 'Wichtig', color: '#EF4444', icon: 'Star' }, +]; +``` + +**Recommendation:** Consider locale-aware default tags or use English defaults that users can customize. + +--- + +#### 3. Database Connection Pattern + +**Location:** `services/mana-core-auth/src/tags/tags.service.ts:21-24` + +```typescript +private getDb() { + const databaseUrl = this.configService.get('database.url'); + return getDb(databaseUrl!); +} +``` + +**Issue:** Using `!` assertion is less safe. + +**Recommendation:** Inject the database connection via NestJS dependency injection instead of calling `getDb()` on every method call. + +--- + +#### 4. Missing Error Boundary Handling + +The NetworkGraph component handles empty states but doesn't have explicit error handling for malformed node/link data. + +**Recommendation:** Add defensive checks for invalid data structures. + +--- + +### Potential Issues + +#### 1. PR Size + +- 382 files is extremely large for a single PR +- Makes code review difficult and increases risk +- Consider splitting into feature branches for easier review and rollback + +#### 2. Database Schema Consistency + +**Location:** `services/mana-core-auth/src/db/schema/tags.schema.ts:11` + +```typescript +userId: text('user_id').notNull(), +``` + +This uses `TEXT` type for user_id. Verify this aligns with how user IDs are stored in other tables (some use `UUID`). + +#### 3. Missing Test Coverage + +This major PR adds significant functionality without visible test changes. Consider adding: +- Unit tests for `TagsService` +- Component tests for `NetworkGraph` +- Integration tests for the themes API + +--- + +### Security Considerations + +#### Authorization Checks ✅ + +- Tag operations properly scope by `userId` +- Custom themes store requires authentication for write operations +- Community theme browsing allows public access (appropriate) + +#### Input Validation + +- DTOs should be reviewed for proper validation (max lengths, format checks) +- Tag color field accepts any 7-char string - consider validating hex format (`/^#[0-9A-Fa-f]{6}$/`) + +#### Environment Files ✅ + +- `.env.development` only removes 6 lines, no secrets added +- No credentials exposed in the diff + +--- + +## New Shared Packages + +| Package | Purpose | +|---------|---------| +| `@manacore/shared-tags` | Client for central tags API | +| `@manacore/shared-help-content` | Markdown content loader and search | +| `@manacore/shared-help-ui` | Svelte help page components | +| `@manacore/shared-help-types` | TypeScript types for help system | +| `@manacore/shared-help-mobile` | React Native help components | +| `@manacore/shared-theme-ui` | Theme editor and community gallery | + +--- + +## Files Changed by Category + +| Category | Count | Notable Changes | +|----------|-------|-----------------| +| Contacts App | ~40 | Duplicates, batch ops, network, favorites | +| Todo App | ~30 | Kanban, statistics, settings, PWA | +| Calendar App | ~25 | Event tags, network graph | +| Shared UI | ~30 | NetworkGraph, skeleton loaders, tags | +| Shared Theme | ~15 | Custom themes store, editor | +| Shared Help | ~35 | Content, UI, types, mobile | +| mana-core-auth | ~15 | Tags API, themes API | +| Archived Apps | ~100 | Documentation cleanup | + +--- + +## Recommendations + +### 1. Split Future PRs + +Consider creating separate PRs for major features: +- Network Graph feature +- Central Tags API +- Custom Themes System +- Help System packages + +### 2. Add i18n + +Replace hardcoded German strings in shared components. + +### 3. Add Tests + +At minimum, add unit tests for: +- `TagsService` (create, update, delete, defaults) +- `ThemesService` (publish, download, rate) +- `NetworkGraph` (props, events, accessibility) + +### 4. Database Migration Plan + +Ensure `tags` and `themes` table migrations are coordinated across environments. + +### 5. Documentation Updates + +The CLAUDE.md files are helpful. Ensure README updates for new packages. + +--- + +## Rating Summary + +| Aspect | Rating | Notes | +|--------|--------|-------| +| Code Quality | ⭐⭐⭐⭐ | Clean, well-structured code | +| Architecture | ⭐⭐⭐⭐⭐ | Excellent use of shared packages | +| Test Coverage | ⭐⭐ | Missing tests for new features | +| PR Size | ⭐⭐ | Too large for single review | +| Security | ⭐⭐⭐⭐ | Good authorization patterns | +| i18n | ⭐⭐⭐ | Some hardcoded German strings | + +--- + +## Conclusion + +This is solid, well-architected code with good separation of concerns. The main concerns are: + +1. **PR size** - Makes review difficult +2. **Missing tests** - New features lack test coverage +3. **Hardcoded strings** - Some German text in shared components + +Consider splitting future releases of this scale into smaller, focused PRs. + +--- + +## Test Plan Checklist + +From the PR description: + +- [ ] Verify network graph loads correctly in Contacts, Calendar, Todo +- [ ] Test theme editor and community themes page +- [ ] Check Todo app new features (kanban, statistics, settings) +- [ ] Verify contacts duplicate detection and batch operations +- [ ] Test skeleton loaders appear during loading states + +--- + +*Review by Claude Code - 2025-12-10*