feat: add announcements system and improve user dashboard layout

[mchestr]

Summary

• Add announcements management system for admins (create, edit, delete with markdown support)
• Display announcements at the top of the user dashboard (hidden when none exist)
• Refactor user dashboard into modular card components (PlexLinkCard, RequestsCard, DiscordCard, WrappedCard, AnnouncementsCard)
• Update Plex card to show dynamic server name ("Access {serverName}")
• Add Announcements nav item to admin sidebar

Test plan

• Verify admin can create, edit, and delete announcements
• Verify announcements appear at top of user dashboard
• Verify dashboard hides announcements section when none exist
• Verify Plex card displays server name correctly
• Verify all unit tests pass

🤖 Generated with Claude Code

[claude]

Code Review - PR #205: Announcements System

Overview

This PR adds a comprehensive announcements management system with admin controls and user-facing display. The implementation is solid overall with good adherence to project conventions. Below are detailed findings organized by severity.

---

🔴 Critical Issues

1. Missing Database Migration

Location: prisma/schema.prisma:665-678

The Announcement model has been added to the schema, but I don't see a corresponding migration file in the PR. This will cause deployment failures.

Action Required:

npm run db:migrate

Make sure to commit the generated migration file.

---

🟡 Important Improvements

2. Missing XSS Protection for Markdown Rendering

Location: components/dashboard/announcements-card.tsx:79-81

The component uses ReactMarkdown to render user-generated content, but there's no sanitization or configuration to prevent XSS attacks. While Markdown libraries generally escape HTML, it's best practice to explicitly configure this.

Recommendation:

<ReactMarkdown
  components={{
    // Disable HTML in markdown to prevent XSS
    html: () => null
  }}
>
  {announcement.content}
</ReactMarkdown>

Or use react-markdown with rehype-sanitize:

npm install rehype-sanitize

3. Race Condition in Toggle Operation

Location: actions/announcements.ts:247-272

The toggleAnnouncementActive function has a potential race condition - it reads the current state, then updates it. If two requests happen simultaneously, both could read the same state and toggle incorrectly.

Fix:

// Use atomic update with Prisma's increment/decrement pattern
await prisma.$executeRaw`
  UPDATE "Announcement" 
  SET "isActive" = NOT "isActive"
  WHERE id = ${id}
`

Or retrieve the updated value:

const updated = await prisma.announcement.update({
  where: { id },
  data: { isActive: { not: true } },
  select: { isActive: true }
})

4. No Test Coverage for New Features

Location: Entire feature

The PR adds 1,474 lines of code but no dedicated tests for the announcements functionality. Only existing tests were updated for navigation changes.

Recommendation: Add tests for:

• Server actions (create, update, delete, toggle)
• AnnouncementsCard component rendering
• AnnouncementsPageClient CRUD operations
• Edge cases (expired announcements, empty states)

5. Missing Error Handling for Date Parsing

Location: actions/announcements.ts:142, 204

The code converts strings to dates without validation:

expiresAt: expiresAt ? new Date(expiresAt) : null,

If an invalid date string is passed, this creates an Invalid Date object that will cause downstream issues.

Fix:

const validateDate = (dateStr: string | null | undefined): Date | null => {
  if (!dateStr) return null
  const date = new Date(dateStr)
  if (isNaN(date.getTime())) {
    throw new Error('Invalid date format')
  }
  return date
}

// Usage:
expiresAt: validateDate(expiresAt)

Or add Zod validation:

expiresAt: z.string().datetime().nullable().optional()

---

🟢 Minor Suggestions

6. Component Size

Location: components/admin/announcements/announcements-page-client.tsx

At 443 lines, this component is approaching the recommended 200-300 line limit mentioned in CLAUDE.md. Consider extracting:

• Form modal into AnnouncementFormModal.tsx
• Announcement list item into AnnouncementListItem.tsx

7. Hardcoded Content Length Limits

Location: components/dashboard/announcements-card.tsx:232

<p className="mt-2 text-slate-300 text-sm whitespace-pre-wrap line-clamp-3">

The line-clamp-3 is hardcoded. If announcements vary in importance, you might want to show high-priority ones in full.

8. Missing data-testid Attributes

Location: components/dashboard/announcements-card.tsx

Per CLAUDE.md E2E testing best practices, interactive elements should have data-testid attributes. The user-facing announcement cards don't have them.

Recommendation:

<motion.article
  key={announcement.id}
  data-testid={`user-announcement-${announcement.id}`}
  // ...
>

9. Inconsistent Priority Sorting

Location: actions/announcements.ts:37-40

orderBy: [
  { priority: "desc" },
  { createdAt: "desc" },
],

Announcements with the same priority will be sorted by creation date (newest first), but this might not be intuitive. Consider documenting this behavior or adding an explicit order field.

10. No Audit Logging

Location: actions/announcements.ts

The code logs operations but doesn't use the audit log system mentioned in CLAUDE.md security section. For admin actions like creating/deleting announcements, audit logs are recommended.

Recommendation:

import { auditLog } from '@/lib/security/audit-log'

// After successful creation:
await auditLog({
  userId: session.user.id,
  action: 'announcement.create',
  resourceType: 'announcement',
  resourceId: announcement.id,
  metadata: { title }
})

---

✅ Strengths

1. Excellent code organization - Server actions, components, and types are well-separated
2. Proper auth checks - All admin-only actions verify session?.user?.isAdmin
3. Good error handling - Try-catch blocks with user-friendly messages
4. Zod validation - Input validation on all server actions
5. Revalidation - Proper use of revalidatePath() after mutations
6. Accessibility - Modal has proper ARIA attributes
7. Responsive design - Mobile-first approach with proper breakpoints
8. Logging - Good use of centralized logger
9. Type safety - Excellent TypeScript usage throughout
10. Existing UI components - Correctly uses StyledInput and ConfirmModal

---

📋 Summary

Overall Assessment: Good implementation with solid architecture. The main concern is the missing migration and potential XSS vulnerability in markdown rendering.

Before Merge:

• Add database migration file
• Add XSS protection to ReactMarkdown
• Fix race condition in toggle function
• Add basic test coverage

Post-Merge:

• Consider refactoring large component
• Add audit logging for admin actions
• Improve date validation

Security: ⚠️ Requires attention (markdown sanitization)
Tests: ❌ No new tests added
Performance: ✅ No concerns
Code Quality: ✅ Follows conventions well

[claude]

Code Review: Announcements System

I've thoroughly reviewed this PR and the implementation looks solid overall. Here's my detailed feedback:

✅ Strengths

1. Excellent Architecture

   • Follows the project's conventions perfectly (Server Actions, proper auth checks, Zod validation)
   • Great modular design - dashboard refactored into reusable card components
   • Proper separation of concerns between client and server components

2. Security

   • All Server Actions correctly verify admin permissions (session?.user?.isAdmin)
   • Comprehensive input validation with Zod schemas
   • Uses Prisma parameterized queries (SQL injection protection)
   • Audit logging via createLogger for admin actions

3. User Experience

   • Markdown support for rich announcements
   • Priority/expiration features provide flexibility
   • Empty state handling (announcements hidden when none exist)
   • Good loading/error states with toast notifications

4. Database Design

   • Proper indexes on isActive/expiresAt and priority for query performance
   • Sensible defaults and nullable fields

5. Testing

   • Updated existing tests to account for new nav items
   • Good use of data-testid attributes throughout

---

🐛 Potential Issues

1. Date Validation Vulnerability (actions/announcements.ts:142, 205)

Issue: expiresAt accepts any string without date validation before converting to Date.

expiresAt: expiresAt ? new Date(expiresAt) : null,

Risk: Invalid date strings (e.g., "invalid") will create Invalid Date objects, causing database errors or unexpected behavior.

Fix: Add Zod date validation:

expiresAt: z.string().datetime().nullable().optional()
// or
expiresAt: z.string().refine((val) => !val || !isNaN(Date.parse(val)), {
  message: "Invalid date format"
}).nullable().optional()

---

2. Race Condition in Toggle (actions/announcements.ts:247-262)

Issue: toggleAnnouncementActive uses two separate operations:

const announcement = await prisma.announcement.findUnique({ where: { id } })
// ...
await prisma.announcement.update({
  where: { id },
  data: { isActive: !announcement.isActive },
})

Risk: If two admins toggle simultaneously, both read the same initial state and write the same final state instead of toggling twice.

Fix: Use atomic update:

const announcement = await prisma.announcement.update({
  where: { id },
  data: { isActive: { set: undefined } }, // read current value
})

await prisma.announcement.update({
  where: { id },
  data: { isActive: !announcement.isActive },
})

Or simpler - combine into one operation if you don't need the return value synchronously.

Impact: Low severity (admin-only, rare scenario), but worth fixing for correctness.

---

3. Missing Error Case Handling (actions/announcements.ts:229, 197)

Issue: deleteAnnouncement and updateAnnouncement don't handle non-existent IDs gracefully.

await prisma.announcement.delete({ where: { id } }) // Throws if not found

Risk: Generic "Failed to delete announcement" error doesn't tell admin if the announcement doesn't exist vs. actual error.

Fix: Check existence first or catch specific Prisma errors:

try {
  await prisma.announcement.delete({ where: { id } })
} catch (error) {
  if (error.code === 'P2025') {
    return { success: false, error: "Announcement not found" }
  }
  throw error
}

---

⚠️ Code Quality Concerns

4. Duplication in Zod Schemas

createAnnouncementSchema and updateAnnouncementSchema are nearly identical. Consider:

const baseAnnouncementSchema = z.object({
  title: z.string().min(1, "Title is required").max(200, "Title too long"),
  content: z.string().min(1, "Content is required").max(5000, "Content too long"),
  priority: z.number().int().min(0).max(100),
  isActive: z.boolean(),
  expiresAt: z.string().datetime().nullable().optional(),
})

const createAnnouncementSchema = baseAnnouncementSchema.extend({
  priority: z.number().int().min(0).max(100).default(0),
  isActive: z.boolean().default(true),
})

const updateAnnouncementSchema = baseAnnouncementSchema.extend({
  id: z.string().min(1),
})

---

5. Potential XSS via Markdown (components/dashboard/announcements-card.tsx:80)

Issue: Using react-markdown without sanitization configuration.

<ReactMarkdown>{announcement.content}</ReactMarkdown>

Risk: While react-markdown sanitizes by default, it's best practice to explicitly configure allowed elements.

Recommendation: Add rehype-sanitize plugin or restrict allowed components:

<ReactMarkdown
  components={{
    // Disable dangerous elements
    script: () => null,
    iframe: () => null,
  }}
>
  {announcement.content}
</ReactMarkdown>

Impact: Low (admin-only content), but defense in depth is good practice.

---

🎨 Minor Improvements

6. Component Size (announcements-page-client.tsx: 443 lines)

   • File is approaching the 300-line guideline mentioned in CLAUDE.md
   • Consider extracting the modal form into a separate component (AnnouncementFormModal)

7. Missing Test Coverage

   • No unit tests for new Server Actions (e.g., actions/announcements.ts)
   • No E2E tests for announcement CRUD flows
   • Consider adding tests following the patterns in components/__tests__/

8. Accessibility

   • Modal has good ARIA attributes (role="dialog", aria-modal)
   • Consider adding aria-describedby for modal description
   • Toggle button icons should have aria-label for screen readers

9. Performance

   • getAllAnnouncements is called on every page load (announcements-page-client.tsx:49)
   • Consider using TanStack Query for caching instead of manual state management

---

📋 Recommendations

Must Fix (Before Merge)

• Fix date validation (chore(deps): bump actions/setup-node from 4 to 6 #1) - prevents database errors

Should Fix (High Priority)

• Fix race condition in toggle (chore(deps): bump docker/build-push-action from 5 to 6 #2)
• Improve error messages for non-existent resources (chore(deps): bump actions/checkout from 4 to 5 #3)

Nice to Have (Future PR)

• Add comprehensive test coverage (chore(deps): bump tailwind-merge from 2.6.0 to 3.4.0 #7)
• Refactor large component (chore(deps-dev): bump tailwindcss from 3.4.18 to 4.1.17 #6)
• Add explicit markdown sanitization config (chore(deps): bump react-dom and @types/react-dom #5)
• Reduce schema duplication (chore(deps): bump node from 20-alpine to 25-alpine #4)

---

🎯 Test Plan Additions

Suggest adding these to the test plan:

• Verify expired announcements are hidden from user dashboard
• Verify priority sorting (higher priority shown first)
• Verify invalid expiration dates are rejected
• Verify concurrent toggles work correctly
• Verify markdown links render correctly and safely

---

📊 Overall Assessment

Code Quality: ⭐⭐⭐⭐ (4/5)
Security: ⭐⭐⭐⭐ (4/5)
Architecture: ⭐⭐⭐⭐⭐ (5/5)

This is well-architected code that follows project conventions excellently. The issues identified are minor and mostly edge cases. The date validation fix is the only critical item before merge.

Great work on the modular dashboard refactoring and clean separation of concerns! 🚀

[claude]

Code Review - PR #205: Announcements System

Overall Assessment

This is a well-implemented feature that follows the project's conventions and best practices. The announcements system is cleanly designed with proper separation of concerns, good security practices, and excellent UX. The dashboard refactoring improves modularity significantly.

---

✅ Strengths

1. Architecture & Code Quality

• ✅ Proper use of Server Actions over API routes (following CLAUDE.md)
• ✅ Clean separation: Server Actions in actions/, components properly split
• ✅ Server components by default with 'use client' only where needed
• ✅ Good component composition - dashboard refactored into modular cards
• ✅ File sizes are reasonable and follow single responsibility principle

2. Security

• ✅ All Server Actions properly check session?.user?.isAdmin
• ✅ Zod validation on all inputs with clear error messages
• ✅ Auth checks before authorization-required operations
• ✅ Proper use of revalidatePath to update cache after mutations
• ✅ Safe markdown rendering with ReactMarkdown (XSS protection)
• ✅ Audit logging for all admin actions

3. Data Layer

• ✅ Proper Prisma schema with composite indexes for performance
• ✅ Migration file follows naming conventions
• ✅ Efficient queries with proper select statements (avoiding over-fetching)
• ✅ Good use of composite index on isActive + expiresAt for filtering
• ✅ Timestamps handled correctly (Date → ISO string conversion)

4. UX & Accessibility

• ✅ Excellent use of data-testid attributes for testing (follows Playwright best practices)
• ✅ Proper ARIA attributes (role="dialog", aria-modal, aria-labelledby)
• ✅ Loading states with spinners
• ✅ Empty states with helpful messaging
• ✅ Toast notifications for user feedback
• ✅ Responsive design with Tailwind utilities
• ✅ Markdown support for rich content formatting

5. UI Consistency

• ✅ Uses existing UI components (StyledInput, ConfirmModal)
• ✅ Consistent Tailwind styling with project theme
• ✅ Framer Motion animations match existing patterns
• ✅ Proper use of gradient backgrounds and borders

---

⚠️ Issues Found

Critical: Missing Test Coverage

Severity: High

The project requires comprehensive test coverage (see CLAUDE.md testing strategy), but this PR adds no tests for:

• Server Actions (actions/announcements.ts)
• Admin UI component (announcements-page-client.tsx)
• User-facing component (announcements-card.tsx)

Required Tests:

1. Server Actions Tests (actions/__tests__/announcements.test.ts)

• Test getActiveAnnouncements filters inactive/expired announcements
• Test getAllAnnouncements requires admin auth
• Test createAnnouncement validates input and requires admin
• Test updateAnnouncement validates input and requires admin
• Test deleteAnnouncement requires admin auth
• Test toggleAnnouncementActive toggles correctly
• Test Zod validation errors are properly returned
• Mock prisma and getServerSession

2. Component Tests

// components/admin/announcements/__tests__/announcements-page-client.test.tsx
- Render with empty state
- Render with announcements list
- Create new announcement (form submission)
- Edit existing announcement
- Delete with confirmation
- Toggle active/inactive status
- Show expired badge correctly
- Mock Server Actions

// components/dashboard/__tests__/announcements-card.test.tsx
- Render empty state
- Render with announcements
- Render markdown correctly
- Show relative dates
- Animations work

References:

• Testing strategy: CLAUDE.md (lines 408-450)
• Existing test patterns: components/__tests__/admin-shared.test.tsx

---

Moderate: Minor Code Quality Issues

1. Error Handling in getActiveAnnouncements (actions/announcements.ts:25)

// Current: Silently returns empty array on error
catch (error) {
  logger.error("Error fetching active announcements", error)
  return []
}

Issue: Users won't see announcements if there's a DB error, but also won't know why.

Recommendation: This might be acceptable for non-critical announcements, but consider whether the homepage should show an error indicator if announcement fetching fails.

2. Missing Validation on Date Logic (announcements-page-client.tsx:156-159)

const isExpired = (expiresAt: string | null) => {
  if (!expiresAt) return false
  return new Date(expiresAt) < new Date()
}

Issue: Client-side expiration check doesn't account for timezone differences. Server filters by expiresAt > now (UTC), but client shows "Expired" badge based on browser time.

Recommendation: Either:

• Accept minor timezone discrepancies (current approach - probably fine)
• Pass server time to client for consistent checks
• Remove client-side expired badge (rely on server filtering)

3. Markdown Content Length (actions/announcements.ts:111)

content: z.string().min(1, "Content is required").max(5000, "Content too long")

Question: 5000 character limit seems arbitrary. Consider if this should match UI constraints or be database-driven. The textarea shows markdown preview but doesn't enforce this limit client-side.

Recommendation: Add maxLength={5000} to textarea for better UX.

---

Minor: Performance Optimizations

1. Announcements Card Scrolling (announcements-card.tsx:64)

<div className="max-h-64 overflow-y-auto">

If there are many announcements, consider:

• Virtual scrolling for 20+ items
• Pagination in admin view
• Limit displayed announcements on dashboard (e.g., top 3 by priority)

Current implementation is fine for typical usage (<10 announcements).

2. Dynamic Import Loading State (announcements-card.tsx:7-9)

const ReactMarkdown = dynamic(() => import("react-markdown"), {
  loading: () => <div className="h-4 w-24 animate-pulse rounded bg-slate-700" />
})

This loading state may flash briefly. Consider preloading markdown on announcements page or using SSR.

---

📋 Pre-Merge Checklist

Before merging, please ensure:

• Add comprehensive test coverage (critical - see above)
• Run npm test and verify all tests pass
• Run npm run lint and fix any issues
• Run npm run build and verify no errors
• Test the feature manually:
  • Create announcement as admin
  • Edit announcement (change expiration, priority, status)
  • Toggle active/inactive status
  • Delete announcement with confirmation
  • Verify announcements appear on user dashboard
  • Verify inactive/expired announcements don't show to users
  • Test markdown rendering (bold, italic, links)
  • Test responsive layout on mobile
• Verify database migration runs successfully: npm run db:migrate

---

🎯 Suggestions for Future Enhancements

(Not required for this PR, but consider for follow-ups)

1. Rich Text Editor: Replace textarea with a markdown editor with preview
2. Scheduling: Add publishAt field for scheduled announcements
3. User Dismissal: Let users dismiss announcements (persist in localStorage or DB)
4. Announcement Types: Add severity levels (info, warning, critical) with color coding
5. Analytics: Track which announcements get the most views
6. E2E Tests: Add Playwright tests for the complete announcement flow

---

📝 Final Notes

This is high-quality work that demonstrates good understanding of the project architecture. The main blocker for merging is test coverage - once tests are added, this will be ready to merge.

The dashboard refactoring is excellent and sets a good pattern for future component extraction. The use of data-testid attributes throughout shows thoughtful consideration for testability.

Estimated effort to address issues:

• Test coverage: ~2-3 hours
• Minor fixes: ~30 minutes

Great job overall! 🚀

[claude]

Code Review Summary

This is a solid implementation. Code follows project conventions well.

Strengths

• Well-structured server actions with Zod validation
• Good database design with appropriate indexes
• Modular component architecture
• Proper accessibility attributes
• Excellent dashboard refactoring

Critical Issues (before merge)

1. Race condition in toggleAnnouncementActive - use explicit boolean parameter instead of read-then-write
2. Missing input validation for expiresAt datetime and ID parameters
3. Missing unit tests for server actions per CLAUDE.md conventions

Medium Issues

• Missing component tests
• Prisma error handling not specific (check P2025 code)
• Component file size 443 lines (approaching limit)

Minor Issues

• FormData interface shadows global FormData
• Inconsistent date formatting
• No focus trap on modal
• No pagination on admin list

Recommendation

Approve with changes. Fix critical issues before merge. Great work on the modular refactoring!

[claude]

Code Review: Announcements System and Dashboard Improvements

Overview

This PR successfully adds an announcements management system and refactors the user dashboard into modular components. The implementation follows the project's conventions and demonstrates good engineering practices overall.

✅ Strengths

Architecture & Design

• Proper separation of concerns: Server Actions in actions/announcements.ts, admin UI client component, and user-facing display component
• Server Components by default: Correctly uses Server Components for the admin page with client component only where needed
• Good component modularity: Dashboard refactored into focused, single-responsibility components (PlexLinkCard, RequestsCard, etc.)
• Type safety: Comprehensive Zod validation and TypeScript interfaces throughout

Database Design

• Well-indexed schema: Composite index on (isActive, expiresAt) and priority index support efficient querying
• Proper timestamps: Includes createdAt, updatedAt, and expiresAt for full lifecycle tracking
• Nullable createdBy: Allows for system-generated announcements if needed

Security

• Auth checks: All mutations properly verify admin status via getServerSession(authOptions)
• Input validation: Zod schemas validate all inputs with sensible limits (title max 200, content max 5000)
• No SQL injection risk: Uses Prisma parameterized queries throughout
• Proper revalidation: Calls revalidatePath() after mutations to invalidate cache

User Experience

• Conditional rendering: Announcements only shown when they exist (no empty state cluttering UI)
• Markdown support: Dynamic import of react-markdown with loading state
• Animations: Smooth framer-motion animations enhance polish
• Responsive design: Mobile-first approach with proper breakpoints

⚠️ Issues & Concerns

1. CRITICAL: Missing Test Coverage

The PR adds significant new functionality but no unit tests for the server actions or components.

Required tests:

• actions/tests/announcements.test.ts - Test all server actions
• components/tests/announcements-card.test.tsx - Test card rendering
• components/admin/announcements/tests/announcements-page-client.test.tsx - Test admin UI

Per CLAUDE.md: Comprehensive coverage - Test all states (loading, error, success, edge cases)

2. Missing E2E Tests

No Playwright tests for the announcements feature. Consider adding:

• Admin creates/edits/deletes announcement flow
• User views announcements on dashboard
• Announcements disappear when expired

3. Accessibility Issues

In announcements-page-client.tsx:245-247:

• Missing aria-label for screen readers on icon-only buttons
• Icon-only buttons need accessible labels

Recommendation: Add aria-label attributes to all icon buttons

4. Error Handling Gaps

In actions/announcements.ts:28-64:

• Silently returns empty array on database errors
• User has no indication something went wrong
• Consider returning { success: false, error: string } pattern used elsewhere

In announcements-page-client.tsx:45-55:

• Catch block is unreachable because getAllAnnouncements() already catches errors and returns []

5. Potential Performance Issue

In announcements-card.tsx:64:

• Only shows ~3-4 announcements before scrolling required
• No pagination or Show more functionality
• Could be jarring if many announcements exist

Recommendation: Consider limiting to top 3-5 by priority, or add View all link

6. Missing Audit Logging

Per CLAUDE.md: Audit logging - Use @/lib/security/audit-log for admin actions

Announcement CRUD operations are admin actions that should be audited for security compliance.

7. Schema Validation Mismatch

In actions/announcements.ts:108-113:

• expiresAt is both nullable() and optional()
• This is redundant - simplify to just z.string().nullable() or z.string().optional()

8. Missing Data Validation

In actions/announcements.ts:141:

• No validation that expiresAt is a valid date string
• No check that expiration is in the future
• Could create confusing expired announcements immediately

Recommendation: Add Zod refinement to validate future dates

🎯 Recommendations

High Priority

1. Add comprehensive unit tests for server actions and components
2. Add audit logging for all announcement CRUD operations
3. Fix accessibility issues (aria-labels on icon buttons)
4. Validate expiration dates are in the future

Medium Priority

5. Add E2E tests for critical user flows
6. Improve error handling (don't silently return empty arrays)
7. Consider pagination/limits for announcement display

Low Priority

8. Extract repeated Tailwind classes to constants
9. Add data-testid attributes for Playwright
10. Refine Zod schemas (remove redundant nullable/optional)

📊 Test Coverage Gap

Based on CLAUDE.md requirements:

• ✅ Component tests updated for dashboard changes
• ❌ Missing: New tests for announcements.ts server actions
• ❌ Missing: Tests for AnnouncementsCard component
• ❌ Missing: Tests for AnnouncementsPageClient component
• ❌ Missing: E2E tests for announcement management flow

Estimated missing test files: 3-4

✨ Overall Assessment

Code Quality: 8/10 - Well-structured, follows conventions, good type safety

Security: 7/10 - Solid auth and validation, but missing audit logging

Test Coverage: 3/10 - Only updated existing tests, no new tests for new functionality

Recommendation: Request changes - The functionality is solid, but the lack of test coverage is a significant gap for a feature of this scope. Once tests are added and the accessibility issues are addressed, this will be ready to merge.

---

Great work on the modular refactor of the dashboard! The announcements feature is well-implemented from a code structure perspective. Adding comprehensive tests will make this production-ready. 🚀

[claude]

Code Review Summary

This PR adds a comprehensive announcements system and refactors the user dashboard. The implementation is generally solid with good adherence to project conventions. Below are my findings:

✅ Strengths

Architecture & Design

• Excellent refactoring: Dashboard components properly split into modular cards (PlexLinkCard, RequestsCard, DiscordCard, WrappedCard, AnnouncementsCard)
• Server Actions usage: Correctly uses Server Actions for mutations instead of API routes (actions/announcements.ts:120, 180, 218, 242)
• Proper component strategy: Server Components for pages, Client Components only where needed ('use client' directives are appropriate)
• Good colocation: Related functionality kept together

Security & Validation

• Strong input validation: Zod schemas validate all inputs with proper constraints (actions/announcements.ts:108-113, 166-173)
• Authorization checks: All actions properly verify admin status (actions/announcements.ts:122, 184, 220, 244)
• SQL injection protection: Uses Prisma parameterized queries throughout
• Audit logging: Good use of logger for tracking admin actions (actions/announcements.ts:145, 204, 233, 262)

Database Design

• Appropriate indexes: Priority and active status properly indexed (migration.sql:17-20)
• Flexible expiration: Optional expiresAt field with proper NULL handling
• Audit trail: Tracks createdBy, createdAt, updatedAt fields

Code Quality

• TypeScript safety: Proper typing throughout, no 'any' types
• Error handling: Consistent try/catch with user-friendly error messages
• Cache invalidation: Proper revalidatePath calls after mutations (actions/announcements.ts:146-147)
• Accessibility: Good use of ARIA attributes and semantic HTML (announcements-page-client.tsx:291-298)
• Test IDs: Excellent use of data-testid attributes for E2E testing

⚠️ Issues & Recommendations

1. Missing Test Coverage ⚠️ HIGH PRIORITY

The announcements feature has ZERO test coverage:

• ❌ No tests for actions/announcements.ts (273 lines)
• ❌ No tests for AnnouncementsPageClient (443 lines)
• ❌ No tests for AnnouncementsCard (114 lines)
• ❌ No tests for new dashboard cards (PlexLinkCard, RequestsCard, DiscordCard)

Required tests:

// actions/__tests__/announcements.test.ts
- getActiveAnnouncements filters expired and inactive announcements
- createAnnouncement validates input and requires admin
- updateAnnouncement prevents unauthorized access
- deleteAnnouncement removes from database
- toggleAnnouncementActive updates status

// components/dashboard/__tests__/announcements-card.test.tsx
- renders empty state when no announcements
- displays announcements with markdown rendering
- shows priority badges correctly
- formats relative dates properly

2. Raw HTML Input Elements ⚠️ MEDIUM PRIORITY

The modal violates project conventions by using raw HTML inputs instead of UI components:

components/admin/announcements/announcements-page-client.tsx:339-349

<textarea
  id="content"
  // ... raw textarea instead of StyledTextarea
/>

components/admin/announcements/announcements-page-client.tsx:373-381

<input
  type="checkbox"
  // ... raw checkbox instead of UI component
/>

Fix: Use existing UI components from components/ui/:

• Replace <textarea> with <StyledTextarea> (check if exists, or create following existing patterns)
• Replace raw checkbox with a proper UI component (see other checkboxes in codebase)

3. Database Schema - Missing Foreign Key Constraint ⚠️ LOW PRIORITY

prisma/schema.prisma (lines added in PR)

model Announcement {
  createdBy String?  // No foreign key relation to User
}

Issue: The createdBy field stores a user ID but lacks a foreign key constraint. This could lead to orphaned records if users are deleted.

Recommendation:

model Announcement {
  createdBy String?
  creator   User?   @relation(fields: [createdBy], references: [id], onDelete: SetNull)
}

4. Performance Consideration ⚠️ LOW PRIORITY

components/dashboard/announcements-card.tsx:7-9

const ReactMarkdown = dynamic(() => import('react-markdown'), {
  loading: () => <div className="h-4 w-24 animate-pulse rounded bg-slate-700" />,
})

Good: Dynamic import for code splitting
Minor issue: The loading skeleton dimensions (h-4 w-24) are tiny compared to actual markdown content, causing layout shift

Suggestion: Use a more representative skeleton or consider if dynamic import is necessary here (react-markdown is likely needed on first page load anyway)

5. Markdown XSS Protection ℹ️ INFO

components/dashboard/announcements-card.tsx:80

<ReactMarkdown>{announcement.content}</ReactMarkdown>

Status: ✅ Safe (react-markdown escapes by default)
Note: If you ever need to allow HTML, add explicit sanitization with DOMPurify. Current implementation is correct.

6. Code Style Nitpicks ℹ️ INFO

components/admin/announcements/announcements-page-client.tsx:156-159

const isExpired = (expiresAt: string | null) => {
  if (!expiresAt) return false
  return new Date(expiresAt) < new Date()
}

Could be simplified to:

const isExpired = (expiresAt: string | null) => 
  expiresAt ? new Date(expiresAt) < new Date() : false

components/dashboard/announcements-card.tsx:64

<div className="max-h-64 overflow-y-auto">

Consider adding scrollbar styling for consistency with rest of app.

🧪 Test Plan Verification

The PR test plan is incomplete:

• ✅ Manual testing steps defined
• ❌ No automated tests to verify behavior
• ❌ No mention of running existing test suite

Recommendation: Before merging, verify:

npm run test          # All unit tests pass
npm run build         # No TypeScript errors
npm run lint          # No linting issues
npm run test:e2e      # E2E tests pass

📊 Code Quality Metrics

Metric	Status	Notes
TypeScript Safety	✅ Excellent	No 'any' types, proper typing throughout
Security	✅ Excellent	Auth checks, validation, SQL injection protection
Architecture	✅ Excellent	Follows Next.js best practices, proper Server/Client split
Test Coverage	❌ Missing	0% coverage for new code
UI Consistency	⚠️ Needs work	Should use existing UI components
Documentation	✅ Good	Clear function comments, proper JSDoc where needed

🎯 Recommendations for Merge

Must Fix Before Merge:

1. Add comprehensive test coverage for all new functionality
2. Replace raw HTML inputs with existing UI components

Should Fix:

3. Add foreign key constraint to createdBy field
4. Improve skeleton loading dimensions

Nice to Have:

5. Code style improvements (optional)

📝 Overall Assessment

Rating: 7/10 - Good implementation with solid architecture and security, but missing critical test coverage.

The code quality is high and follows project conventions well. The main blocker is the lack of tests, which is essential per the project's comprehensive testing strategy (CLAUDE.md). Once tests are added and UI components are used consistently, this will be ready to merge.

Great work on the refactoring and clean separation of concerns! 🎉

[claude]

Code Review - PR #205: Announcements System

Overall Assessment

This is a well-implemented feature that follows the project's architectural patterns and coding standards. The code quality is high, with proper validation, error handling, and security checks.

Strengths

Architecture & Patterns

• Excellent use of Server Actions for all mutations
• Dashboard refactored into modular components (PlexLinkCard, RequestsCard, DiscordCard, WrappedCard, AnnouncementsCard)
• Correct Server/Client component separation
• Good data fetching patterns with Promise.all()

Security

• All admin actions properly protected with auth checks
• Comprehensive Zod input validation
• Audit logging for all CRUD operations
• XSS protection via ReactMarkdown

Code Quality

• Excellent TypeScript usage with proper typing
• Consistent error handling with logging
• Proper path revalidation after mutations
• Good accessibility (ARIA attributes, semantic HTML)

Database Design

• Well-designed schema with appropriate indexes
• Efficient queries with filtering and sorting
• Clean migration included

UI/UX

• Follows Tailwind conventions, dark theme first
• Responsive mobile-first design
• Good loading/error states
• Proper data-testid attributes for testing

---

Issues Found

CRITICAL - Missing Test Coverage (HIGH)

The announcements feature has ZERO test coverage. CLAUDE.md requires comprehensive tests.

Missing:

1. Server Actions tests (authorization, validation, CRUD, expiration logic)
2. Component tests (AnnouncementsPageClient, AnnouncementsCard)
3. E2E tests (admin creates/edits, users see active announcements)

This is a user-facing feature with admin privileges - tests are essential before merging.

---

MEDIUM - Database Race Condition

Location: actions/announcements.ts:247-262

toggleAnnouncementActive uses read-then-write pattern causing race conditions.

Recommended fix: Let client pass desired state explicitly rather than toggling server-side.

---

LOW - Missing Date Validation

Location: actions/announcements.ts:114

expiresAt accepts string but doesn't validate ISO date format. Add .datetime() validation.

---

LOW - Performance Consideration

Location: announcements-page-client.tsx:46-56

Refetches all announcements after every mutation. Consider optimistic updates (not critical).

---

Code Metrics

• Files Changed: 16
• Lines Added: 1,515
• Lines Removed: 94
• New Components: 6
• Test Coverage: 0%

---

Dashboard Refactoring Excellence

The dashboard refactoring is excellent:

• Before: One 400+ line component
• After: Six focused components (60-270 lines each)
• Improved testability and maintainability

---

Summary

Excellent engineering practices and valuable functionality. Well-structured, secure, follows conventions. Main blocker: missing test coverage.

Recommendation: Request changes for test coverage, then approve.

Great work overall!

[claude]

Pull Request Review: Announcements System

Summary

This PR adds a comprehensive announcements management system with admin CRUD operations and user-facing display. The implementation is well-structured with good separation of concerns, excellent test coverage, and follows the project's architecture patterns.

---

✅ Strengths

1. Excellent Test Coverage

• Comprehensive unit tests for all server actions (377 lines in announcements.test.ts)
• Tests cover authorization, validation, error handling, and edge cases
• Proper mocking of dependencies (Prisma, NextAuth, revalidatePath)
• Tests for Prisma P2025 error handling

2. Strong Security

• Admin-only operations properly protected with session checks in actions/announcements.ts
• Authorization checks occur before any database operations
• Audit logging for all CRUD operations with user tracking
• Security warning logs for unauthorized access attempts

3. Architecture Adherence

• Follows CLAUDE.md conventions consistently:
  • Server Actions for mutations (not API routes) ✅
  • Zod validation on all inputs ✅
  • Proper error handling without throwing in Server Actions ✅
  • TypeScript strict mode compliance ✅
  • Centralized logger usage ✅

4. Good UX Design

• Progressive disclosure: announcements only shown when they exist
• Responsive design with mobile-first approach
• Markdown support for rich content formatting
• Priority-based sorting ensures important announcements are visible
• Optional expiration dates for time-sensitive announcements
• Real-time toggle for active/inactive status

5. Database Design

• Well-structured schema with proper indexes:
  • Composite index on (isActive, expiresAt) for efficient filtering
  • Priority index for sorting performance
• Proper field constraints and defaults
• Tracks createdBy for audit trail

---

🔧 Issues & Recommendations

Critical Issues

1. Missing Foreign Key Constraint (prisma/schema.prisma)

Location: Migration file and schema
Issue: The createdBy field doesn't have a foreign key relationship to the User table.

// Current - in migration
"createdBy" TEXT,

// Should be
"createdBy" TEXT NOT NULL,
CONSTRAINT "Announcement_createdBy_fkey" FOREIGN KEY ("createdBy") REFERENCES "User"("id") ON DELETE SET NULL ON UPDATE CASCADE

Impact:

• Data integrity risk - could reference non-existent users
• No cascading delete behavior defined
• createdBy should probably be NOT NULL since we always have a session

Recommendation: Add a foreign key relationship in the schema and create a follow-up migration.

2. XSS Risk with Markdown Rendering (announcements-card.tsx:80)

Location: components/dashboard/announcements-card.tsx
Issue: ReactMarkdown is dynamically imported and renders user content without explicit sanitization configuration.

<ReactMarkdown>{announcement.content}</ReactMarkdown>

Impact:

• If an admin account is compromised, malicious markdown could be injected
• ReactMarkdown has some built-in protections, but explicit sanitization is best practice

Recommendation:

import rehypeSanitize from 'rehype-sanitize'

<ReactMarkdown rehypePlugins={[rehypeSanitize]}>
  {announcement.content}
</ReactMarkdown>

---

High Priority Issues

3. Client-Side State Duplication (announcements-page-client.tsx)

Location: components/admin/announcements/announcements-page-client.tsx:38-55
Issue: Component maintains local state (announcements) and manually calls loadAnnouncements() after mutations, rather than using cache revalidation.

const [announcements, setAnnouncements] = useState<AnnouncementData[]>(initialAnnouncements)

const loadAnnouncements = useCallback(async () => {
  setLoading(true)
  try {
    const data = await getAllAnnouncements()
    setAnnouncements(data)
  } catch {
    toast.showError("Failed to load announcements")
  } finally {
    setLoading(false)
  }
}, [toast])

Impact:

• Unnecessary network requests
• State management complexity
• Doesn't leverage Next.js built-in caching

Recommendation: Per CLAUDE.md guidelines, use TanStack Query for client-side data fetching:

const { data: announcements = [], isLoading, refetch } = useQuery({
  queryKey: ['admin', 'announcements'],
  queryFn: getAllAnnouncements,
  initialData: initialAnnouncements
})

// After mutations, use refetch() or invalidate the query

4. Date Validation Logic Flaw (actions/announcements.ts:112-115)

Location: actions/announcements.ts
Issue: Custom date validation accepts any parseable date, including invalid ones like "0000-00-00" or distant past dates.

const isValidDateString = (val: string): boolean => {
  const date = new Date(val)
  return !isNaN(date.getTime())
}

Impact:

• Could accept semantically invalid dates
• No validation that expiration is in the future

Recommendation:

const createAnnouncementSchema = z.object({
  // ...
  expiresAt: z.string()
    .datetime() // Built-in ISO datetime validation
    .refine((val) => new Date(val) > new Date(), { 
      message: "Expiration must be in the future" 
    })
    .nullable()
    .optional(),
})

---

Medium Priority Issues

5. Missing E2E Tests

Issue: No Playwright tests for the announcements feature.

Impact:

• Can't verify the full user flow (create → display → edit → delete)
• UI interactions not tested

Recommendation: Add E2E test coverage:

• Admin creates announcement
• Verify announcement appears on user dashboard
• Edit and delete operations
• Use data-testid attributes (already present ✅)

6. Duplicate Revalidation Calls (actions/announcements.ts)

Issue: Every mutation calls both revalidatePath('/') and revalidatePath('/admin/announcements').

revalidatePath("/")
revalidatePath("/admin/announcements")

Impact: Minor performance cost (2 revalidations per operation).

Recommendation: This is fine for now, but consider if you need both. The / revalidation handles the dashboard, which is the primary use case.

7. Landing Page Simplification May Be Too Aggressive (app/(app)/page.tsx)

Issue: The PR drastically simplifies the landing page, removing feature highlights and the detailed hero section.

Before: Detailed hero with feature highlights, server name, and onboarding messaging
After: Simple server name + sign-in button

Impact:

• Loss of user onboarding information
• Less context for new users
• Removing AdminFooter component (may be intentional)

Recommendation: Verify this simplification is intentional. If so, consider keeping some minimal feature highlights for first-time visitors.

---

Low Priority / Nitpicks

8. Modal Accessibility (announcements-page-client.tsx:293-303)

Good: Modal has proper ARIA attributes (role="dialog", aria-modal, aria-labelledby)
Missing:

• Focus trap implementation (user can tab outside modal)
• Escape key handler (user can't close modal with keyboard)

Recommendation:

// Add escape key handler
useEffect(() => {
  const handleEscape = (e: KeyboardEvent) => {
    if (e.key === 'Escape' && (showCreateModal || editingAnnouncement)) {
      closeModal()
    }
  }
  window.addEventListener('keydown', handleEscape)
  return () => window.removeEventListener('keydown', handleEscape)
}, [showCreateModal, editingAnnouncement])

9. Inconsistent Date Formatting

Issue: Different date formatting approaches across components:

• announcements-card.tsx: Relative dates ("Today", "2 days ago")
• announcements-page-client.tsx: Absolute dates (toLocaleDateString())

Recommendation: Extract to a shared utility function for consistency.

10. Missing Loading States

Issue: The AnnouncementsCard doesn't show loading state while markdown is being parsed.

Current:

const ReactMarkdown = dynamic(() => import("react-markdown"), {
  loading: () => <div className="h-4 w-24 animate-pulse rounded bg-slate-700" />,
})

Issue: The loading placeholder is tiny (h-4 w-24) and doesn't match the content size.

Recommendation: Use a skeleton that matches the content layout.

---

📊 Code Quality Metrics

Aspect	Rating	Notes
Architecture	⭐⭐⭐⭐⭐	Follows CLAUDE.md patterns perfectly
Test Coverage	⭐⭐⭐⭐⭐	Comprehensive unit tests (377 lines)
Security	⭐⭐⭐⭐	Good auth, missing FK constraint & XSS hardening
Type Safety	⭐⭐⭐⭐⭐	Full TypeScript, Zod validation
Performance	⭐⭐⭐⭐	Good indexes, minor optimization opportunities
Accessibility	⭐⭐⭐⭐	Good ARIA, missing focus trap
UX	⭐⭐⭐⭐⭐	Clean, intuitive, responsive

---

🎯 Recommended Action Items

Before Merge:

1. ✅ Fix foreign key constraint (critical for data integrity)
2. ✅ Add markdown sanitization (security)
3. ✅ Improve date validation (use z.datetime())

Post-Merge:

4. Consider migrating to TanStack Query for client state
5. Add E2E test coverage
6. Add escape key handler for modal
7. Verify landing page simplification is intentional

---

🏆 Overall Assessment

Verdict: Approve with Minor Changes Required

This is a high-quality PR that demonstrates strong understanding of the project's architecture and conventions. The implementation is clean, well-tested, and follows best practices. The issues identified are mostly minor refinements rather than fundamental flaws.

The feature itself is valuable and well-designed. Once the critical security/integrity issues are addressed, this will be a solid addition to the codebase.

Estimated effort to address critical items: ~30 minutes

Great work! 🎉