branderson/dotcom-arcade
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: code review issues

Browse Source
This commit is contained in:
Darkraise
2026-02-04 16:57:48 +07:00
parent 855cb8b38c
commit 971b432d94
6 changed files with 201 additions and 158 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.prettierignore
+1
View File
@@ -65,6 +65,7 @@ Thumbs.db
!game-service/src/**/*.d.ts
# Documentation (preserve original formatting)
docs/
CHANGELOG.md
LICENSE
backend/src/migrations/README.md
+54 -50
View File
@@ -2,21 +2,24 @@
This directory contains SQL migration scripts for the user database (`user.db`).
## Current Migration Strategy (Simplified)
## Current Migration Strategy
**Date:** 2026-01-30 **Status:** Consolidated into single migration file
The database uses a **consolidated base schema plus incremental migrations**
approach:
All previous migrations have been consolidated into a single comprehensive
migration file:
- **`001_complete_schema.sql`** - Consolidated base schema with all core tables,
indexes, triggers, and seed data
- **`002_domains.sql`** - Adds the `domains` table for configurable domain
settings
- **`001_complete_schema.sql`** - Complete database schema with all tables,
indexes, triggers, seed data
The base schema (`001`) was consolidated from 14+ earlier migrations on
2026-01-30. New features added after that point are shipped as incremental
migration files (`002`, `003`, etc.). All migrations are **idempotent** and safe
to run multiple times.
This migration is **idempotent** and safe to run multiple times.
### What's Included in `001_complete_schema.sql`:
### What's Included:
✅ **All Tables** (15 total):
**All Core Tables** (16 total):
- Users & Authentication (users, roles, permissions, role_permissions,
refresh_tokens)
@@ -25,25 +28,25 @@ This migration is **idempotent** and safe to run multiple times.
- User Data (user_settings, user_playlists, user_playlist_games, user_favorites)
- System (system_settings, job_execution_logs)
✅ **All Indexes** (50+ optimized indexes):
**All Indexes** (50+ optimized indexes):
- Basic indexes for foreign keys and lookups
- Composite indexes for complex queries
- Partial indexes for conditional queries
- Performance indexes for frequently-accessed patterns
✅ **All Triggers**:
**All Triggers**:
- Auto-update playlist game_count
✅ **All Seed Data**:
**All Seed Data**:
- Default roles (admin, user, guest)
- Default permissions (18 permissions)
- Role-permission mappings
- System settings (35+ default settings)
✅ **Playlist Sharing Features**:
**Playlist Sharing Features**:
- share_token, share_expires_at, show_owner columns
- Unique indexes for secure sharing
@@ -53,24 +56,24 @@ This migration is **idempotent** and safe to run multiple times.
Migrations are automatically executed on server startup by
`UserDatabaseService.ts`:
1. **Bootstrap** - Creates migration registry table (`migrations_applied`)
2. **Schema Creation** - Runs `001_complete_schema.sql` if database doesn't
exist
3. **Idempotent** - Safe to run multiple times (uses `IF NOT EXISTS` everywhere)
1. **Bootstrap** - Creates migration registry table (`migrations`)
2. **Detection** - Scans for `NNN_*.sql` files and checks the registry for
already-applied entries
3. **Execution** - Runs pending migrations in version order
4. **Idempotent** - Safe to run multiple times (uses `IF NOT EXISTS` everywhere)
### Migration Registry (`bootstrap.sql`)
The migration system tracks applied migrations in the `migrations_applied`
table:
The migration system tracks applied migrations in the `migrations` table:
```sql
CREATE TABLE IF NOT EXISTS migrations_applied (
CREATE TABLE IF NOT EXISTS migrations (
id INTEGER PRIMARY KEY AUTOINCREMENT,
version TEXT NOT NULL UNIQUE,
name TEXT NOT NULL,
checksum TEXT NOT NULL,
executed_at TEXT DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')),
execution_time_ms INTEGER
name TEXT NOT NULL UNIQUE,
applied_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')),
checksum TEXT,
execution_time_ms INTEGER,
description TEXT
);
```
@@ -106,15 +109,15 @@ For new columns, indexes, or seed data:
```bash
cd backend/src/migrations
touch 002_your_migration_name.sql
touch 003_your_migration_name.sql
```
Current version: **001** (next available: **002**)
Current latest version: **002** (next available: **003**)
2. **Write idempotent SQL** using defensive checks:
```sql
-- Migration 002: Description of changes
-- Migration 003: Description of changes
-- Created: 2026-MM-DD
-- Add new column (defensive)
@@ -127,14 +130,9 @@ For new columns, indexes, or seed data:
INSERT OR IGNORE INTO system_settings (key, value, ...) VALUES (...);
```
3. **Update `UserDatabaseService.ts`** to run the new migration:
```typescript
// In runMigrations() method, after the 001 migration:
// Migration 002: Your description
await this.executeMigration('002_your_migration_name.sql');
```
3. **No code changes needed.** `UserDatabaseService.runMigrations()` auto-scans
the migrations directory for `NNN_*.sql` files, checks the `migrations`
registry table, and runs any that haven't been applied yet.
4. **Test the migration**:
- Backup `backend/user.db`
@@ -146,8 +144,8 @@ For new columns, indexes, or seed data:
For breaking changes or complete restructuring:
1. Create a new comprehensive migration (e.g., `002_schema_v2.sql`)
2. Archive the old migration (`001_complete_schema.sql` → `archived/`)
1. Create a new comprehensive migration (e.g., `003_schema_v2.sql`)
2. Archive the old base migration (`001_complete_schema.sql` → `archived/`)
3. Update documentation
## Migration Best Practices
@@ -176,7 +174,7 @@ For breaking changes or complete restructuring:
```sql
-- ============================================================================
-- Migration 002: Add User Notification Preferences
-- Migration 003: Add User Notification Preferences
-- ============================================================================
-- Created: 2026-MM-DD
-- Purpose: Add email notification settings for users
@@ -276,14 +274,17 @@ If you see checksum warnings:
- Migration file was modified after being applied
- This is usually safe if changes are additive
- Check `migrations_applied` table for recorded checksum
- Check `migrations` table for recorded checksum
## Database Schema Overview
After running `001_complete_schema.sql`, the database contains:
After running all migrations, the database contains:
```
user.db
├── migrations - Migration tracking (bootstrap.sql)
│
│ 001_complete_schema.sql:
├── users - User accounts
├── roles - RBAC roles (admin, user, guest)
├── permissions - Available permissions
@@ -300,10 +301,13 @@ user.db
├── user_playlist_games - Playlist-game relationships
├── user_favorites - Favorited games
├── job_execution_logs - Background job tracking
└── migrations_applied - Migration tracking (bootstrap.sql)
│
│ 002_domains.sql:
└── domains - Configurable domains for sharing & CORS
```
**Total:** 16 tables, 50+ indexes, 2 triggers, 35+ default settings
**Total:** 17 tables (+1 registry), 50+ indexes, 2 triggers, 35+ default
settings
## System Settings Categories
@@ -337,14 +341,14 @@ Previous migration strategy used 14+ separate files:
- `003-013` - Incremental changes
- `014_add_performance_indexes.sql` - Performance optimization
**Consolidated into:** Single `001_complete_schema.sql` file
**Consolidated into:** `001_complete_schema.sql` as the base schema. New
features added after consolidation are shipped as incremental migrations
(`002_domains`, etc.).
**Benefits:**
- ✅ Simpler deployment (one file vs many)
- ✅ Guaranteed consistency (all-or-nothing)
- ✅ Easier testing (single execution)
- ✅ Clearer dependencies (everything in order)
- ✅ Better documentation (complete picture in one place)
- Simpler base deployment (one file covers all core tables)
- Guaranteed consistency for the base schema (all-or-nothing)
- Incremental migrations for new features (standard forward-only approach)
**Archived migrations** preserved in `archived/` for reference.
backend/src/routes/domains.ts
+126 -89
View File
@@ -1,10 +1,11 @@
import { Router, Request, Response } from 'express';
import { Router } from 'express';
import { z } from 'zod';
import { DomainService } from '../services/DomainService';
import { authenticate } from '../middleware/auth';
import { requirePermission } from '../middleware/rbac';
import { logActivity } from '../middleware/activityLogger';
import { logger } from '../utils/logger';
import { asyncHandler } from '../middleware/asyncHandler';
import { AppError } from '../middleware/errorHandler';
const router = Router();
const domainService = DomainService.getInstance();
@@ -16,24 +17,112 @@ router.get(
'/',
authenticate,
requirePermission('settings.read'),
async (req: Request, res: Response) => {
try {
const domains = domainService.getAllDomains();
res.json(domains);
} catch (error) {
logger.error('Failed to get domains:', error);
res.status(500).json({
error: { message: 'Failed to retrieve domains' },
});
}
}
asyncHandler(async (_req, res) => {
const domains = domainService.getAllDomains();
res.json(domains);
})
);
// ===================================
// ADD DOMAIN
// ===================================
const hostnameSchema = z
.string()
.trim()
.min(1, 'Hostname is required')
.max(253, 'Hostname must not exceed 253 characters')
.superRefine((val, ctx) => {
// Reject protocol prefixes
if (/^[a-z][a-z0-9+.-]*:\/\//i.test(val)) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: 'Hostname must not include a protocol (e.g., http:// or https://)',
});
return;
}
// Reject whitespace anywhere
if (/\s/.test(val)) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: 'Hostname must not contain whitespace',
});
return;
}
// Reject wildcards
if (val.includes('*')) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: 'Hostname must not contain wildcards',
});
return;
}
// Reject paths, query strings, fragments
if (/[/?#]/.test(val)) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: 'Hostname must not include a path, query string, or fragment',
});
return;
}
// Reject userinfo (user:pass@host)
if (val.includes('@')) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: 'Hostname must not include userinfo (user@host)',
});
return;
}
// Split host and optional port
const portMatch = val.match(/^(.+):(\d+)$/);
const host = portMatch ? portMatch[1] : val;
const port = portMatch ? parseInt(portMatch[2], 10) : null;
// Validate port range
if (port !== null && (port < 1 || port > 65535)) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: 'Port must be between 1 and 65535',
});
return;
}
// Validate hostname: alphanumeric labels separated by dots, hyphens allowed (not at start/end of label)
const labels = host.split('.');
const labelPattern = /^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/i;
for (const label of labels) {
if (!label) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: 'Hostname must not contain empty labels (consecutive dots)',
});
return;
}
if (label.length > 63) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: 'Each hostname label must not exceed 63 characters',
});
return;
}
if (!labelPattern.test(label)) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message:
'Hostname labels must contain only letters, digits, and hyphens, and must not start or end with a hyphen',
});
return;
}
}
});
const addDomainSchema = z.object({
hostname: z.string().min(1, 'Hostname is required'),
hostname: hostnameSchema,
});
router.post(
@@ -41,38 +130,18 @@ router.post(
authenticate,
requirePermission('settings.update'),
logActivity('domains.add', 'domains'),
async (req: Request, res: Response) => {
asyncHandler(async (req, res) => {
try {
const validation = addDomainSchema.safeParse(req.body);
if (!validation.success) {
return res.status(400).json({
error: {
message: 'Invalid request body',
details: validation.error.errors,
},
});
}
const domain = domainService.addDomain(validation.data.hostname, req.user!.id);
const data = addDomainSchema.parse(req.body);
const domain = domainService.addDomain(data.hostname, req.user!.id);
res.status(201).json(domain);
} catch (error) {
if (
error instanceof Error &&
(error.message.includes('already exists') ||
error.message.includes('must not') ||
error.message.includes('Invalid') ||
error.message.includes('cannot be empty'))
) {
return res.status(400).json({
error: { message: error.message },
});
if (error instanceof z.ZodError) {
throw new AppError(400, `Validation error: ${error.errors[0].message}`);
}
logger.error('Failed to add domain:', error);
res.status(500).json({
error: { message: 'Failed to add domain' },
});
throw error;
}
}
})
);
// ===================================
@@ -83,31 +152,15 @@ router.delete(
authenticate,
requirePermission('settings.update'),
logActivity('domains.delete', 'domains'),
async (req: Request, res: Response) => {
try {
const id = parseInt(req.params.id, 10);
if (isNaN(id)) {
return res.status(400).json({
error: { message: 'Invalid domain ID' },
});
}
const deleted = domainService.deleteDomain(id);
if (!deleted) {
return res.status(404).json({
error: { message: 'Domain not found' },
});
}
res.json({ success: true });
} catch (error) {
logger.error('Failed to delete domain:', error);
res.status(500).json({
error: { message: 'Failed to delete domain' },
});
asyncHandler(async (req, res) => {
const id = parseInt(req.params.id, 10);
if (isNaN(id)) {
throw new AppError(400, 'Invalid domain ID');
}
}
domainService.deleteDomain(id);
res.json({ success: true });
})
);
// ===================================
@@ -118,31 +171,15 @@ router.patch(
authenticate,
requirePermission('settings.update'),
logActivity('domains.setDefault', 'domains'),
async (req: Request, res: Response) => {
try {
const id = parseInt(req.params.id, 10);
if (isNaN(id)) {
return res.status(400).json({
error: { message: 'Invalid domain ID' },
});
}
const domain = domainService.setDefault(id);
if (!domain) {
return res.status(404).json({
error: { message: 'Domain not found' },
});
}
res.json(domain);
} catch (error) {
logger.error('Failed to set default domain:', error);
res.status(500).json({
error: { message: 'Failed to set default domain' },
});
asyncHandler(async (req, res) => {
const id = parseInt(req.params.id, 10);
if (isNaN(id)) {
throw new AppError(400, 'Invalid domain ID');
}
}
const domain = domainService.setDefault(id);
res.json(domain);
})
);
export default router;
backend/src/services/DomainService.ts
+10 -10
View File
@@ -1,4 +1,5 @@
import { UserDatabaseService } from './UserDatabaseService';
import { AppError } from '../middleware/errorHandler';
import { logger } from '../utils/logger';
export interface Domain {
@@ -86,22 +87,22 @@ export class DomainService {
const trimmed = hostname.trim().toLowerCase();
if (!trimmed) {
throw new Error('Hostname cannot be empty');
throw new AppError(400, 'Hostname cannot be empty');
}
// Reject protocol prefixes
if (/^https?:\/\//i.test(trimmed)) {
throw new Error('Hostname must not include a protocol (http:// or https://)');
throw new AppError(400, 'Hostname must not include a protocol (http:// or https://)');
}
// Reject path, query, or fragment
if (/[/?#]/.test(trimmed)) {
throw new Error('Hostname must not include a path, query string, or fragment');
throw new AppError(400, 'Hostname must not include a path, query string, or fragment');
}
// Basic hostname validation: allow alphanumeric, dots, hyphens, colons (for port)
if (!/^[a-z0-9._-]+(:[0-9]+)?$/i.test(trimmed)) {
throw new Error('Invalid hostname format');
throw new AppError(400, 'Invalid hostname format');
}
return trimmed;
@@ -119,7 +120,7 @@ export class DomainService {
| undefined;
if (existing) {
throw new Error(`Domain "${validated}" already exists`);
throw new AppError(409, `Domain "${validated}" already exists`);
}
const stmt = this.db.prepare('INSERT INTO domains (hostname, created_by) VALUES (?, ?)');
@@ -138,31 +139,30 @@ export class DomainService {
/**
* Delete a domain by ID
*/
deleteDomain(id: number): boolean {
deleteDomain(id: number): void {
const existing = this.db.prepare('SELECT hostname FROM domains WHERE id = ?').get(id) as
| { hostname: string }
| undefined;
if (!existing) {
return false;
throw new AppError(404, 'Domain not found');
}
this.db.prepare('DELETE FROM domains WHERE id = ?').run(id);
this.invalidateCache();
logger.info(`[Domains] Deleted domain "${existing.hostname}" (id: ${id})`);
return true;
}
/**
* Set a domain as the default (clears previous default in a transaction)
*/
setDefault(id: number): Domain | null {
setDefault(id: number): Domain {
const existing = this.db
.prepare('SELECT id, hostname, is_default, created_at FROM domains WHERE id = ?')
.get(id) as DomainRow | undefined;
if (!existing) {
return null;
throw new AppError(404, 'Domain not found');
}
const transaction = this.db.transaction(() => {
docs/07-design-system/README.md
+5 -4
View File
@@ -110,10 +110,11 @@ function Example() {
## Component Categories
**Layout:** Card, Sheet, Dialog, Scroll Area, Separator **Forms:** Input,
Textarea, Select, Checkbox, Radio, Switch, Button **Data:** Table, Badge, Alert,
Avatar, Skeleton **Navigation:** Dropdown Menu, Collapsible, Toggle, Toggle
Group **Feedback:** Toast, Alert Dialog, Progress
- **Layout:** Card, Sheet, Dialog, Scroll Area, Separator
- **Forms:** Input, Textarea, Select, Checkbox, Radio, Switch, Button
- **Data:** Table, Badge, Alert, Avatar, Skeleton
- **Navigation:** Dropdown Menu, Collapsible, Toggle, Toggle Group
- **Feedback:** Toast, Alert Dialog, Progress
## Typography
docs/07-design-system/color-palette.md
+5 -5
View File
@@ -11,12 +11,12 @@ Colors stored as HSL without `hsl()` wrapper to support opacity modifiers:
--primary: 221.2 83.2% 53.3%;
```
Usage:
Usage in JSX:
```tsx
bg-primary; /* hsl(221.2 83.2% 53.3%) */
bg-primary/50; /* hsl(221.2 83.2% 53.3% / 0.5) */
bg-primary/20; /* hsl(221.2 83.2% 53.3% / 0.2) */
<div className="bg-primary" /> {/* hsl(221.2 83.2% 53.3%) */}
<div className="bg-primary/50" /> {/* hsl(221.2 83.2% 53.3% / 0.5) */}
<div className="bg-primary/20" /> {/* hsl(221.2 83.2% 53.3% / 0.2) */}
```
## Neutral Palettes
@@ -180,7 +180,7 @@ const { setMode } = useThemeStore();
// Toggle for testing
<button onClick={() => setMode(mode === 'dark' ? 'light' : 'dark')}>
Toggle Theme
</button>
</button>;
```
### 4. Maintain Consistency