From 167f1b2acb43ded64b5c67df9875d14f3d0cb613 Mon Sep 17 00:00:00 2001 From: Brad Groux Date: Sat, 31 Jan 2026 04:41:50 -0600 Subject: [PATCH] =?UTF-8?q?fix:=20enforce=20global=20timer=20exclusivity?= =?UTF-8?q?=20=E2=80=94=20only=20one=20timer=20at=20a=20time?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Server: startTimer() auto-stops any running timer on another task before starting the new one. Prevents multiple simultaneous timers. - Server: updateTask() re-reads from cache inside file lock to prevent concurrent writes (debounced saves) from clobbering timer state. - Server: Added WebSocket broadcasts to all timer routes (start/stop/add/delete) so other clients get real-time updates. - UI: TimeTrackingSection hides Start button when another task has an active timer, showing 'Timer active on another task' instead. - UI: Subscribe directly to query cache for timer state instead of relying on the debounced-save prop pipeline. - UI: Added mutatingRef guard to prevent double-clicks on Start/Stop. Fixes the issue where 3 tasks could have running timers simultaneously. --- server/src/routes/task-time.ts | 14 ++- server/src/services/task-service.ts | 113 ++++++++++++------ .../components/task/TimeTrackingSection.tsx | 112 ++++++++++------- 3 files changed, 157 insertions(+), 82 deletions(-) diff --git a/server/src/routes/task-time.ts b/server/src/routes/task-time.ts index b0ce6ef4..a49e7712 100644 --- a/server/src/routes/task-time.ts +++ b/server/src/routes/task-time.ts @@ -1,6 +1,7 @@ import { Router, type Router as RouterType } from 'express'; import { z } from 'zod'; import { getTaskService } from '../services/task-service.js'; +import { broadcastTaskChange } from '../services/broadcast-service.js'; import { asyncHandler } from '../middleware/async-handler.js'; import { ValidationError } from '../middleware/error-handler.js'; @@ -26,7 +27,15 @@ router.get( router.post( '/:id/time/start', asyncHandler(async (req, res) => { - const task = await taskService.startTimer(req.params.id as string); + const { task, stoppedTaskId } = await taskService.startTimer(req.params.id as string); + + // Broadcast WS event for the auto-stopped task (if any) so other clients refresh + if (stoppedTaskId) { + broadcastTaskChange('updated', stoppedTaskId); + } + // Broadcast WS event for the started task + broadcastTaskChange('updated', task.id); + res.json(task); }) ); @@ -36,6 +45,7 @@ router.post( '/:id/time/stop', asyncHandler(async (req, res) => { const task = await taskService.stopTimer(req.params.id as string); + broadcastTaskChange('updated', task.id); res.json(task); }) ); @@ -55,6 +65,7 @@ router.post( throw error; } const task = await taskService.addTimeEntry(req.params.id as string, duration, description); + broadcastTaskChange('updated', task.id); res.json(task); }) ); @@ -67,6 +78,7 @@ router.delete( req.params.id as string, req.params.entryId as string ); + broadcastTaskChange('updated', task.id); res.json(task); }) ); diff --git a/server/src/services/task-service.ts b/server/src/services/task-service.ts index 43fcd9a5..4f1b9031 100644 --- a/server/src/services/task-service.ts +++ b/server/src/services/task-service.ts @@ -421,55 +421,69 @@ export class TaskService { } async updateTask(id: string, input: UpdateTaskInput): Promise { + // Initial read to check existence and compute the lock filepath. + // NOTE: this data may be stale by the time we acquire the lock — + // the actual merge happens inside the lock with a fresh cache read. const task = await this.getTask(id); if (!task) return null; - // Track if status changed for telemetry - const previousStatus = task.status; - const statusChanged = input.status !== undefined && input.status !== previousStatus; - // Handle git field separately to merge properly const { git: gitUpdate, blockedReason: blockedReasonUpdate, ...restInput } = input; - const updatedTask: Task = { - ...task, - ...restInput, - git: gitUpdate ? ({ ...task.git, ...gitUpdate } as Task['git']) : task.git, - // Handle blockedReason: null means clear, undefined means keep existing - blockedReason: - blockedReasonUpdate === null ? undefined : (blockedReasonUpdate ?? task.blockedReason), - updated: new Date().toISOString(), - }; - - // Remove old file if title changed (filename changes) + // Compute filenames for locking. We use the tentative updated task + // to determine the new filename (title may have changed). const oldFilename = this.taskToFilename(task); - const newFilename = this.taskToFilename(updatedTask); + const tentativeTask: Task = { ...task, ...restInput }; + const newFilename = this.taskToFilename(tentativeTask); const filepath = path.join(this.tasksDir, newFilename); - const content = this.taskToMarkdown(updatedTask); + + let updatedTask!: Task; await withFileLock(filepath, async () => { + // Re-read from cache inside the lock to get the latest state. + // This prevents concurrent writes (e.g., debounced field save vs. + // timer start) from overwriting each other's changes. + const freshTask = this.cacheGet(id) ?? task; + + const previousStatus = freshTask.status; + const statusChanged = input.status !== undefined && input.status !== previousStatus; + + updatedTask = { + ...freshTask, + ...restInput, + git: gitUpdate ? ({ ...freshTask.git, ...gitUpdate } as Task['git']) : freshTask.git, + // Handle blockedReason: null means clear, undefined means keep existing + blockedReason: + blockedReasonUpdate === null + ? undefined + : (blockedReasonUpdate ?? freshTask.blockedReason), + updated: new Date().toISOString(), + }; + + const content = this.taskToMarkdown(updatedTask); this.markWrite(); + if (oldFilename !== newFilename) { // Intentionally silent: old file may already be gone after rename await fs.unlink(path.join(this.tasksDir, oldFilename)).catch(() => {}); } await fs.writeFile(filepath, content, 'utf-8'); + + // Write-through: update cache immediately (inside lock for consistency) + this.cache.set(updatedTask.id, updatedTask); + + // Emit telemetry event if status changed + if (statusChanged) { + await this.telemetry.emit({ + type: 'task.status_changed', + taskId: updatedTask.id, + project: updatedTask.project, + status: updatedTask.status, + previousStatus, + }); + } }); - // Write-through: update cache immediately - this.cache.set(updatedTask.id, updatedTask); - - // Emit telemetry event if status changed - if (statusChanged) { - await this.telemetry.emit({ - type: 'task.status_changed', - taskId: updatedTask.id, - project: updatedTask.project, - status: updatedTask.status, - previousStatus, - }); - } - return updatedTask; } @@ -666,19 +680,47 @@ export class TaskService { // ============ Time Tracking Methods ============ /** - * Start a timer for a task + * Find the task that currently has a running timer (if any). */ - async startTimer(taskId: string): Promise { + async getRunningTimerTask(): Promise { + await this.initCache(); + for (const task of this.cache.values()) { + if (task.timeTracking?.isRunning) { + return task; + } + } + return null; + } + + /** + * Start a timer for a task. + * Enforces global exclusivity: only one timer may run at a time. + * If another task has a running timer, it is auto-stopped first. + * Returns { task, stoppedTaskId? } so the caller can broadcast both. + */ + async startTimer(taskId: string): Promise<{ task: Task; stoppedTaskId?: string }> { const task = await this.getTask(taskId); if (!task) { throw new Error('Task not found'); } - // Check if timer is already running + // Check if timer is already running on THIS task if (task.timeTracking?.isRunning) { throw new Error('Timer is already running for this task'); } + // Global exclusivity: stop any other running timer first + let stoppedTaskId: string | undefined; + const runningTask = await this.getRunningTimerTask(); + if (runningTask && runningTask.id !== taskId) { + await this.stopTimer(runningTask.id); + stoppedTaskId = runningTask.id; + log.info( + { stoppedTaskId, startedTaskId: taskId }, + 'Auto-stopped timer for global exclusivity' + ); + } + const entryId = `time_${Date.now()}_${Math.random().toString(36).slice(2, 8)}`; const now = new Date().toISOString(); @@ -694,7 +736,8 @@ export class TaskService { activeEntryId: entryId, }; - return this.updateTask(taskId, { timeTracking }) as Promise; + const updated = (await this.updateTask(taskId, { timeTracking })) as Task; + return { task: updated, stoppedTaskId }; } /** diff --git a/web/src/components/task/TimeTrackingSection.tsx b/web/src/components/task/TimeTrackingSection.tsx index 285b1d49..72cc26dc 100644 --- a/web/src/components/task/TimeTrackingSection.tsx +++ b/web/src/components/task/TimeTrackingSection.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useCallback } from 'react'; +import { useState, useEffect, useCallback, useRef } from 'react'; import { useQueryClient } from '@tanstack/react-query'; import { Button } from '@/components/ui/button'; import { Input } from '@/components/ui/input'; @@ -21,6 +21,7 @@ import { formatDuration, parseDuration, } from '@/hooks/useTimeTracking'; +import { useTasks } from '@/hooks/useTasks'; import { Play, Square, Plus, Trash2, Clock, Loader2, Timer } from 'lucide-react'; import type { Task, TimeEntry } from '@veritas-kanban/shared'; import { cn } from '@/lib/utils'; @@ -57,8 +58,6 @@ export function TimeTrackingSection({ task }: TimeTrackingSectionProps) { const [addDialogOpen, setAddDialogOpen] = useState(false); const [durationInput, setDurationInput] = useState(''); const [descriptionInput, setDescriptionInput] = useState(''); - // Local optimistic state — toggles immediately on click, syncs with server data - const [optimisticRunning, setOptimisticRunning] = useState(null); const queryClient = useQueryClient(); const startTimer = useStartTimer(); @@ -66,35 +65,46 @@ export function TimeTrackingSection({ task }: TimeTrackingSectionProps) { const addTimeEntry = useAddTimeEntry(); const deleteTimeEntry = useDeleteTimeEntry(); - const serverRunning = task.timeTracking?.isRunning || false; - // Use optimistic state if set, otherwise fall back to server state - const isRunning = optimisticRunning !== null ? optimisticRunning : serverRunning; - const totalSeconds = task.timeTracking?.totalSeconds || 0; - const entries = task.timeTracking?.entries || []; - const activeEntry = entries.find((e) => e.id === task.timeTracking?.activeEntryId); + // Ref to prevent double-clicks — blocks new clicks until the current + // mutation fully resolves (success or error). React state updates can lag + // behind DOM re-renders, so disabled={isPending} alone isn't enough. + const mutatingRef = useRef(false); - // Sync optimistic state back when server catches up - useEffect(() => { - if (optimisticRunning !== null && serverRunning === optimisticRunning) { - setOptimisticRunning(null); - } - }, [serverRunning, optimisticRunning]); + // Subscribe directly to the tasks query cache so timer state updates + // bypass the useDebouncedSave → localTask prop pipeline (which can lag + // behind due to effect timing). patchTaskInList updates ['tasks'] cache + // immediately on mutation success, so this gives us real-time timer state. + const { data: allTasks } = useTasks(); + const timerTask = allTasks?.find((t) => t.id === task.id) || task; + + const isRunning = timerTask.timeTracking?.isRunning || false; + const totalSeconds = timerTask.timeTracking?.totalSeconds || 0; + const entries = timerTask.timeTracking?.entries || []; + const activeEntry = entries.find((e) => e.id === timerTask.timeTracking?.activeEntryId); + + // Global exclusivity: find if ANY other task has a running timer + const otherRunningTask = allTasks?.find((t) => t.id !== task.id && t.timeTracking?.isRunning); const handleStartStop = useCallback(async () => { - // Toggle immediately for responsive UI - const newState = !isRunning; - setOptimisticRunning(newState); + // Prevent double-clicks via ref (synchronous guard) + if (mutatingRef.current) return; + mutatingRef.current = true; + try { - if (!newState) { + if (isRunning) { await stopTimer.mutateAsync(task.id); } else { await startTimer.mutateAsync(task.id); } - } catch { - // API rejected — clear optimistic state and force-refresh from server - // so the UI reflects the real timer state (not stale cache) - setOptimisticRunning(null); + } catch (err) { + // Server state differs from what we expected — force-refresh from + // server so the UI shows the real timer state. queryClient.invalidateQueries({ queryKey: ['tasks'] }); + // Log for debugging but don't show error to user — the invalidation + // will sync the UI to the correct state automatically. + console.warn('[TimeTracking] mutation failed, syncing from server:', err); + } finally { + mutatingRef.current = false; } }, [isRunning, task.id, startTimer, stopTimer, queryClient]); @@ -127,6 +137,8 @@ export function TimeTrackingSection({ task }: TimeTrackingSectionProps) { }); }; + const isBusy = startTimer.isPending || stopTimer.isPending; + return (
@@ -143,26 +155,34 @@ export function TimeTrackingSection({ task }: TimeTrackingSectionProps) { {/* Timer controls */}
- + {/* Only show Start/Stop when this task's timer is running OR no other timer is active */} + {isRunning ? ( + + ) : !otherRunningTask ? ( + + ) : ( + + Timer active on another task + + )} {isRunning && activeEntry && (
@@ -243,14 +263,14 @@ export function TimeTrackingSection({ task }: TimeTrackingSectionProps) { key={entry.id} className={cn( 'flex items-center justify-between p-2 rounded text-sm', - entry.id === task.timeTracking?.activeEntryId + entry.id === timerTask.timeTracking?.activeEntryId ? 'bg-green-500/10 border border-green-500/20' : 'bg-muted/50' )} >
- {entry.id === task.timeTracking?.activeEntryId ? ( + {entry.id === timerTask.timeTracking?.activeEntryId ? ( ) : ( @@ -272,7 +292,7 @@ export function TimeTrackingSection({ task }: TimeTrackingSectionProps) { : formatEntryTime(entry)}
- {entry.id !== task.timeTracking?.activeEntryId && ( + {entry.id !== timerTask.timeTracking?.activeEntryId && (