diff --git a/node_modules b/node_modules deleted file mode 120000 index 87273d9f..00000000 --- a/node_modules +++ /dev/null @@ -1 +0,0 @@ -/home/bill/volvox-bot/node_modules \ No newline at end of file diff --git a/tests/modules/commandAliases.test.js b/tests/modules/commandAliases.test.js index 4798adda..c9f671a2 100644 --- a/tests/modules/commandAliases.test.js +++ b/tests/modules/commandAliases.test.js @@ -2,7 +2,7 @@ * Tests for src/modules/commandAliases.js */ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; vi.mock('../../src/logger.js', () => ({ info: vi.fn(), diff --git a/web/src/app/api/guilds/[guildId]/ai-feedback/stats/route.ts b/web/src/app/api/guilds/[guildId]/ai-feedback/stats/route.ts index fbe95b48..084a5682 100644 --- a/web/src/app/api/guilds/[guildId]/ai-feedback/stats/route.ts +++ b/web/src/app/api/guilds/[guildId]/ai-feedback/stats/route.ts @@ -35,9 +35,18 @@ export async function GET( ); if (upstreamUrl instanceof NextResponse) return upstreamUrl; - const days = request.nextUrl.searchParams.get('days'); - if (days !== null) { - upstreamUrl.searchParams.set('days', days); + // Validate and clamp 'days' param to prevent unbounded/expensive lookback queries + const rawDays = request.nextUrl.searchParams.get('days'); + if (rawDays !== null) { + const parsed = parseInt(rawDays, 10); + if (Number.isNaN(parsed)) { + return NextResponse.json( + { error: 'Invalid days parameter: must be an integer' }, + { status: 400 }, + ); + } + const clampedDays = Math.min(90, Math.max(1, parsed)); + upstreamUrl.searchParams.set('days', String(clampedDays)); } return proxyToBotApi( diff --git a/web/src/components/dashboard/ai-feedback-stats.tsx b/web/src/components/dashboard/ai-feedback-stats.tsx index 6ba8d1f9..8bfdbc63 100644 --- a/web/src/components/dashboard/ai-feedback-stats.tsx +++ b/web/src/components/dashboard/ai-feedback-stats.tsx @@ -17,19 +17,8 @@ import { } from 'recharts'; import { Card, CardContent, CardDescription, CardHeader, CardTitle } from '@/components/ui/card'; import { useGuildSelection } from '@/hooks/use-guild-selection'; -import { getBotApiBaseUrl } from '@/lib/bot-api'; - -interface FeedbackStats { - positive: number; - negative: number; - total: number; - ratio: number | null; - trend: Array<{ - date: string; - positive: number; - negative: number; - }>; -} + +import type { AiFeedbackStats as AiFeedbackStatsType } from '@/types/analytics'; const PIE_COLORS = ['#22C55E', '#EF4444']; @@ -38,41 +27,43 @@ const PIE_COLORS = ['#22C55E', '#EF4444']; * Shows 👍/👎 aggregate counts, approval ratio, and daily trend. */ export function AiFeedbackStats() { - const selectedGuild = useGuildSelection(); - const [stats, setStats] = useState(null); + const guildId = useGuildSelection(); + const [stats, setStats] = useState(null); const [loading, setLoading] = useState(false); const [error, setError] = useState(null); const fetchStats = useCallback(async () => { - const apiBase = getBotApiBaseUrl(); - if (!selectedGuild || !apiBase) return; + if (!guildId) return; setLoading(true); setError(null); try { - const res = await fetch(`${apiBase}/guilds/${selectedGuild}/ai-feedback/stats?days=30`, { - credentials: 'include', - }); + const res = await fetch( + `/api/guilds/${encodeURIComponent(guildId)}/ai-feedback/stats?days=30`, + { + credentials: 'include', + }, + ); if (!res.ok) { throw new Error(`HTTP ${res.status}`); } - const data = (await res.json()) as FeedbackStats; + const data = (await res.json()) as AiFeedbackStats; setStats(data); } catch (err) { setError(err instanceof Error ? err.message : 'Failed to load feedback stats'); } finally { setLoading(false); } - }, [selectedGuild]); + }, [guildId]); useEffect(() => { void fetchStats(); }, [fetchStats]); - if (!selectedGuild) return null; + if (!guildId) return null; const pieData = stats && stats.total > 0 @@ -148,11 +139,8 @@ export function AiFeedbackStats() { } labelLine={false} > - {pieData.map((_, index) => ( - + {pieData.map((entry, index) => ( + ))} diff --git a/web/src/components/dashboard/config-diff-modal.tsx b/web/src/components/dashboard/config-diff-modal.tsx new file mode 100644 index 00000000..d1689ebd --- /dev/null +++ b/web/src/components/dashboard/config-diff-modal.tsx @@ -0,0 +1,126 @@ +'use client'; + +import { Loader2, RotateCcw, Save } from 'lucide-react'; +import { Button } from '@/components/ui/button'; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from '@/components/ui/dialog'; +import { ConfigDiff } from './config-diff'; + +interface ConfigDiffModalProps { + /** Whether the modal is open. */ + open: boolean; + /** Callback to open/close the modal. Blocked while saving. */ + onOpenChange: (open: boolean) => void; + /** The original (saved) config to diff against. */ + original: object; + /** The modified (draft) config to diff. */ + modified: object; + /** Top-level section keys that have changes. */ + changedSections: string[]; + /** Called when user confirms the save. */ + onConfirm: () => void; + /** Called when user reverts a specific top-level section. */ + onRevertSection: (section: string) => void; + /** Whether a save is in progress. */ + saving: boolean; +} + +/** + * A modal dialog that shows a diff preview of pending config changes before saving. + * + * Displays the changed sections as badges with individual revert buttons, a scrollable + * line-by-line diff, and Cancel / Confirm Save actions. + * + * @param open - Whether the dialog is visible. + * @param onOpenChange - Callback to open/close the dialog (blocked while saving). + * @param original - The original saved config object. + * @param modified - The draft config object with pending changes. + * @param changedSections - List of top-level section keys that differ. + * @param onConfirm - Called when the user clicks "Confirm Save". + * @param onRevertSection - Called with a section key when the user reverts that section. + * @param saving - When true, disables controls and shows a spinner on the confirm button. + * @returns The diff preview dialog element. + */ +export function ConfigDiffModal({ + open, + onOpenChange, + original, + modified, + changedSections, + onConfirm, + onRevertSection, + saving, +}: ConfigDiffModalProps) { + return ( + + + + Review Changes Before Saving + + Review your pending changes. Revert individual sections or confirm to save all changes. + + + + {/* Changed sections with per-section revert buttons */} + {changedSections.length > 0 && ( +
+ Changed sections + + {changedSections.map((section) => ( +
+ + {section} + + +
+ ))} +
+ )} + + {/* Scrollable diff view */} +
+ +
+ + + + + + + + ); +} diff --git a/web/src/components/dashboard/config-editor.tsx b/web/src/components/dashboard/config-editor.tsx index 335c170c..325ba8c0 100644 --- a/web/src/components/dashboard/config-editor.tsx +++ b/web/src/components/dashboard/config-editor.tsx @@ -1,6 +1,6 @@ 'use client'; -import { AlertCircle, CheckCircle2, Loader2, RefreshCw } from 'lucide-react'; +import { Loader2, RotateCcw, Save } from 'lucide-react'; import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { toast } from 'sonner'; import { Button } from '@/components/ui/button'; @@ -12,6 +12,7 @@ import { GUILD_SELECTED_EVENT, SELECTED_GUILD_KEY } from '@/lib/guild-selection' import type { BotConfig, DeepPartial } from '@/types/config'; import { SYSTEM_PROMPT_MAX_LENGTH } from '@/types/config'; import { ConfigDiff } from './config-diff'; +import { ConfigDiffModal } from './config-diff-modal'; import { DiscardChangesButton } from './reset-defaults-button'; import { SystemPromptEditor } from './system-prompt-editor'; @@ -123,6 +124,11 @@ export function ConfigEditor() { const [guildId, setGuildId] = useState(''); const [loading, setLoading] = useState(false); const [saving, setSaving] = useState(false); + const [showDiffModal, setShowDiffModal] = useState(false); + const [prevSavedConfig, setPrevSavedConfig] = useState<{ + guildId: string; + config: GuildConfig; + } | null>(null); const [error, setError] = useState(null); /** The config as last fetched from the API (the "saved" state). */ @@ -136,15 +142,6 @@ export function ConfigEditor() { const abortRef = useRef(null); - /** Auto-save status indicator. */ - const [saveStatus, setSaveStatus] = useState<'idle' | 'saving' | 'saved' | 'error'>('idle'); - /** Debounce timer for auto-save. */ - const autoSaveTimerRef = useRef | null>(null); - /** True while the initial config load is in progress — suppresses auto-save. */ - const isLoadingConfigRef = useRef(false); - /** Ref tracking latest draftConfig for race-condition detection during save. */ - const draftConfigRef = useRef(null); - const updateDraftConfig = useCallback((updater: (prev: GuildConfig) => GuildConfig) => { setDraftConfig((prev) => updater((prev ?? {}) as GuildConfig)); }, []); @@ -178,7 +175,7 @@ export function ConfigEditor() { }, []); // ── Load config when guild changes ───────────────────────────── - const fetchConfig = useCallback(async (id: string, { skipSaveStatusReset = false } = {}) => { + const fetchConfig = useCallback(async (id: string) => { if (!id) return; abortRef.current?.abort(); @@ -187,7 +184,6 @@ export function ConfigEditor() { setLoading(true); setError(null); - isLoadingConfigRef.current = true; try { const res = await fetch(`/api/guilds/${encodeURIComponent(id)}/config`, { @@ -219,19 +215,10 @@ export function ConfigEditor() { } setSavedConfig(data); setDraftConfig(structuredClone(data)); - // Reset status after a successful reload; clear any stale error - if (!skipSaveStatusReset) { - setSaveStatus('idle'); - } - // Use a macrotask so the state setter for draftConfig fires before we clear the flag - setTimeout(() => { - isLoadingConfigRef.current = false; - }, 0); setDmStepsRaw((data.welcome?.dmSequence?.steps ?? []).join('\n')); setProtectRoleIdsRaw((data.moderation?.protectRoles?.roleIds ?? []).join(', ')); } catch (err) { if ((err as Error).name === 'AbortError') return; - isLoadingConfigRef.current = false; const msg = (err as Error).message || 'Failed to load config'; setError(msg); toast.error('Failed to load config', { description: msg }); @@ -245,11 +232,6 @@ export function ConfigEditor() { return () => abortRef.current?.abort(); }, [guildId, fetchConfig]); - // Keep draftConfigRef in sync for race-condition detection in saveChanges - useEffect(() => { - draftConfigRef.current = draftConfig; - }, [draftConfig]); - // ── Derived state ────────────────────────────────────────────── const hasChanges = useMemo(() => { if (!savedConfig || !draftConfig) return false; @@ -271,6 +253,12 @@ export function ConfigEditor() { return promptLength > SYSTEM_PROMPT_MAX_LENGTH; }, [draftConfig]); + /** Top-level config sections that have pending changes. */ + const changedSections = useMemo(() => { + if (!savedConfig || !draftConfig) return []; + const patches = computePatches(savedConfig, draftConfig); + return [...new Set(patches.map((p) => p.path.split('.')[0]))]; + }, [savedConfig, draftConfig]); // ── Warn on unsaved changes before navigation ────────────────── useEffect(() => { if (!hasChanges) return; @@ -285,7 +273,47 @@ export function ConfigEditor() { }, [hasChanges]); // ── Save changes (batched: parallel PATCH per section) ───────── - const saveChanges = useCallback(async () => { + // ── Open diff modal before saving ───────────────────────────── + const openDiffModal = useCallback(() => { + if (!guildId || !savedConfig || !draftConfig) return; + if (hasValidationErrors) { + toast.error('Cannot save', { + description: 'Fix validation errors before saving.', + }); + return; + } + if (!hasChanges) { + toast.info('No changes to save.'); + return; + } + setShowDiffModal(true); + }, [guildId, savedConfig, draftConfig, hasValidationErrors, hasChanges]); + + // ── Revert a single top-level section to saved state ────────── + const revertSection = useCallback( + (section: string) => { + if (!savedConfig) return; + setDraftConfig((prev) => { + if (!prev) return prev; + return { + ...prev, + [section]: (savedConfig as Record)[section], + } as GuildConfig; + }); + // Keep raw string mirrors consistent + if (section === 'welcome') { + setDmStepsRaw((savedConfig.welcome?.dmSequence?.steps ?? []).join('\n')); + } + if (section === 'moderation') { + setProtectRoleIdsRaw((savedConfig.moderation?.protectRoles?.roleIds ?? []).join(', ')); + } + toast.success(`Reverted ${section} changes.`); + }, + [savedConfig], + ); + + // ── Execute the save (called from diff modal confirm) ────────── + const executeSave = useCallback(async () => { if (!guildId || !savedConfig || !draftConfig) return; if (hasValidationErrors) { @@ -296,10 +324,11 @@ export function ConfigEditor() { } const patches = computePatches(savedConfig, draftConfig); - if (patches.length === 0) return; - - // Snapshot draft before saving to detect changes made during in-flight save - const draftSnapshot = draftConfig; + if (patches.length === 0) { + setShowDiffModal(false); + toast.info('No changes to save.'); + return; + } // Group patches by top-level section for batched requests const bySection = new Map>(); @@ -314,7 +343,6 @@ export function ConfigEditor() { } setSaving(true); - setSaveStatus('saving'); // Shared AbortController for all section saves - aborts all in-flight requests on 401 const saveAbortController = new AbortController(); @@ -379,70 +407,60 @@ export function ConfigEditor() { return updated; }); } - setSaveStatus('error'); toast.error('Some sections failed to save', { description: `Failed: ${failedSections.join(', ')}`, }); } else { - setSaveStatus('saved'); - // Full success: check if draft changed during save (user edited while saving) - if (deepEqual(draftConfigRef.current, draftSnapshot)) { - // No changes during save — safe to reload authoritative version - await fetchConfig(guildId, { skipSaveStatusReset: true }); - } else { - // Draft changed during save — don't overwrite user's new changes - // Update savedConfig to reflect what was successfully persisted - setSavedConfig(structuredClone(draftSnapshot)); - } + toast.success('Config saved successfully!'); + setShowDiffModal(false); + // Store previous config for undo (1 level deep; scoped to current guild) + setPrevSavedConfig({ guildId, config: structuredClone(savedConfig) as GuildConfig }); + // Full success: reload to get the authoritative version from the server + await fetchConfig(guildId); } } catch (err) { const msg = (err as Error).message || 'Failed to save config'; - setSaveStatus('error'); toast.error('Failed to save config', { description: msg }); } finally { setSaving(false); } }, [guildId, savedConfig, draftConfig, hasValidationErrors, fetchConfig]); - // ── Auto-save: debounce 500ms after draft changes ────────────── + // Clear undo snapshot when guild changes to prevent cross-guild config corruption useEffect(() => { - // Don't auto-save during initial config load or if no changes - if (isLoadingConfigRef.current || !hasChanges || hasValidationErrors || saving) return; + setPrevSavedConfig(null); + }, []); - // Clear any pending timer - if (autoSaveTimerRef.current) { - clearTimeout(autoSaveTimerRef.current); + // ── Undo last save ───────────────────────────────────────────── + const undoLastSave = useCallback(() => { + if (!prevSavedConfig) return; + // Guard: discard snapshot if guild changed since save + if (prevSavedConfig.guildId !== guildId) { + setPrevSavedConfig(null); + return; } + setDraftConfig(structuredClone(prevSavedConfig.config)); + setDmStepsRaw((prevSavedConfig.config.welcome?.dmSequence?.steps ?? []).join('\n')); + setProtectRoleIdsRaw( + (prevSavedConfig.config.moderation?.protectRoles?.roleIds ?? []).join(', '), + ); + setPrevSavedConfig(null); + toast.info('Reverted to previous saved state. Save again to apply.'); + }, [prevSavedConfig, guildId]); - autoSaveTimerRef.current = setTimeout(() => { - void saveChanges(); - }, 500); - - return () => { - if (autoSaveTimerRef.current) { - clearTimeout(autoSaveTimerRef.current); - } - }; - }, [hasChanges, hasValidationErrors, saving, saveChanges]); - - // ── Keyboard shortcut: Ctrl/Cmd+S to save ────────────────────── + // ── Keyboard shortcut: Ctrl/Cmd+S → open diff preview ───────── useEffect(() => { function onKeyDown(e: KeyboardEvent) { if ((e.metaKey || e.ctrlKey) && e.key === 's') { e.preventDefault(); if (hasChanges && !saving && !hasValidationErrors) { - // Cancel any pending debounce timer and save immediately - if (autoSaveTimerRef.current) { - clearTimeout(autoSaveTimerRef.current); - autoSaveTimerRef.current = null; - } - void saveChanges(); + openDiffModal(); } } } window.addEventListener('keydown', onKeyDown); return () => window.removeEventListener('keydown', onKeyDown); - }, [hasChanges, saving, hasValidationErrors, saveChanges]); + }, [hasChanges, saving, hasValidationErrors, openDiffModal]); // ── Discard edits ────────────────────────────────────────────── const discardChanges = useCallback(() => { @@ -757,24 +775,61 @@ export function ConfigEditor() { Manage AI, welcome messages, and other settings.

-
- {/* Auto-save status indicator */} - +
+ {/* Undo last save — visible only after a successful save with no new changes */} + {prevSavedConfig && !hasChanges && ( + + )} + {/* Save button with unsaved-changes indicator dot */} +
+ + {hasChanges && !saving && ( +
- {/* Validation error banner */} - {hasChanges && hasValidationErrors && ( + {/* Unsaved changes banner */} + {hasChanges && ( - Fix validation errors before changes can be saved. + You have unsaved changes.{' '} + + Ctrl+S + {' '} + to save. )} @@ -2281,61 +2336,23 @@ export function ConfigEditor() { - {/* Diff view */} + {/* Inline diff view — shows pending changes below the form */} {hasChanges && savedConfig && } - - ); -} - -// ── Auto-Save Status Indicator ──────────────────────────────── - -interface AutoSaveStatusProps { - status: 'idle' | 'saving' | 'saved' | 'error'; - onRetry: () => void; -} -/** - * Displays the current auto-save status with an icon and text. - * - * Shows nothing when idle, a spinner while saving, a check icon on success, - * and an error state with a retry button on failure. - */ -function AutoSaveStatus({ status, onRetry }: AutoSaveStatusProps) { - if (status === 'idle') return null; - - if (status === 'saving') { - return ( - - - ); - } - - if (status === 'saved') { - return ( - - - ); - } - - // error state - return ( - - + {/* Diff modal — shown before saving to require explicit confirmation */} + {savedConfig && ( + + )} + ); } diff --git a/web/src/types/analytics.ts b/web/src/types/analytics.ts index 45ebf668..b030d007 100644 --- a/web/src/types/analytics.ts +++ b/web/src/types/analytics.ts @@ -81,3 +81,16 @@ export interface DashboardAnalytics { messages: number; }>; } + +/** Shape of the /guilds/:id/ai-feedback/stats API response. */ +export interface AiFeedbackStats { + positive: number; + negative: number; + total: number; + ratio: number | null; + trend: Array<{ + date: string; + positive: number; + negative: number; + }>; +} diff --git a/web/tests/components/dashboard/config-diff-modal.test.tsx b/web/tests/components/dashboard/config-diff-modal.test.tsx new file mode 100644 index 00000000..878d3d08 --- /dev/null +++ b/web/tests/components/dashboard/config-diff-modal.test.tsx @@ -0,0 +1,74 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { ConfigDiffModal } from '@/components/dashboard/config-diff-modal'; + +const baseProps = { + open: true, + onOpenChange: vi.fn(), + original: { ai: { enabled: false }, welcome: { enabled: true } }, + modified: { ai: { enabled: true }, welcome: { enabled: true } }, + changedSections: ['ai'], + onConfirm: vi.fn(), + onRevertSection: vi.fn(), + saving: false, +}; + +describe('ConfigDiffModal', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('renders the dialog when open', () => { + render(); + expect(screen.getByText('Review Changes Before Saving')).toBeInTheDocument(); + }); + + it('shows changed sections as badges', () => { + render(); + expect(screen.getByText('ai')).toBeInTheDocument(); + expect(screen.getByText('welcome')).toBeInTheDocument(); + }); + + it('calls onRevertSection with section name when revert button clicked', async () => { + const user = userEvent.setup(); + const onRevertSection = vi.fn(); + render(); + await user.click(screen.getByRole('button', { name: 'Revert ai changes' })); + expect(onRevertSection).toHaveBeenCalledWith('ai'); + }); + + it('calls onConfirm when "Confirm Save" button clicked', async () => { + const user = userEvent.setup(); + const onConfirm = vi.fn(); + render(); + await user.click(screen.getByRole('button', { name: /confirm save/i })); + expect(onConfirm).toHaveBeenCalledTimes(1); + }); + + it('calls onOpenChange(false) when Cancel is clicked', async () => { + const user = userEvent.setup(); + const onOpenChange = vi.fn(); + render(); + await user.click(screen.getByRole('button', { name: /cancel/i })); + expect(onOpenChange).toHaveBeenCalledWith(false); + }); + + it('disables buttons while saving', () => { + render(); + expect(screen.getByRole('button', { name: /cancel/i })).toBeDisabled(); + expect(screen.getByRole('button', { name: /saving/i })).toBeDisabled(); + expect(screen.getByRole('button', { name: 'Revert ai changes' })).toBeDisabled(); + }); + + it('shows "Saving..." text while saving', () => { + render(); + expect(screen.getByRole('button', { name: /saving/i })).toBeInTheDocument(); + }); + + it('does not render changed sections fieldset when changedSections is empty', () => { + render(); + // No badges rendered + expect(screen.queryByText('ai')).not.toBeInTheDocument(); + }); +}); diff --git a/web/tests/components/dashboard/config-diff.test.tsx b/web/tests/components/dashboard/config-diff.test.tsx new file mode 100644 index 00000000..2d0725e7 --- /dev/null +++ b/web/tests/components/dashboard/config-diff.test.tsx @@ -0,0 +1,39 @@ +import { describe, expect, it } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import { ConfigDiff } from '@/components/dashboard/config-diff'; + +describe('ConfigDiff', () => { + it('shows "no changes" when objects are identical', () => { + const config = { ai: { enabled: true }, welcome: { enabled: false } }; + render(); + expect(screen.getByText('No changes detected.')).toBeInTheDocument(); + }); + + it('renders added/removed line counts when configs differ', () => { + const original = { ai: { enabled: false } }; + const modified = { ai: { enabled: true } }; + render(); + // Added/removed counts should be visible + expect(screen.getByText(/\+\d+/)).toBeInTheDocument(); + expect(screen.getByText(/-\d+/)).toBeInTheDocument(); + }); + + it('uses custom title when provided', () => { + const config = { ai: { enabled: true } }; + render(); + expect(screen.getByText('My Custom Title')).toBeInTheDocument(); + }); + + it('defaults to "Pending Changes" title', () => { + const config = { ai: { enabled: true } }; + render(); + expect(screen.getByText('Pending Changes')).toBeInTheDocument(); + }); + + it('shows diff view when configs differ', () => { + const original = { ai: { enabled: false } }; + const modified = { ai: { enabled: true } }; + render(); + expect(screen.getByRole('region', { name: 'Configuration diff' })).toBeInTheDocument(); + }); +});