scriptoriumadmin/veritas-kanban
1
0
Fork 0
mirror of https://github.com/BradGroux/veritas-kanban.git synced 2026-10-09 05:07:53 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

fix: enforce global timer exclusivity — only one timer at a time

Browse source 
- 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.
This commit is contained in:
Brad Groux 2026-01-31 04:41:50 -06:00
parent bf61fd698b
commit 167f1b2acb
3 changed files with 157 additions and 82 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
server/src/routes/task-time.ts
View file

@ -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);
})
);

113
server/src/services/task-service.ts
View file

@ -421,55 +421,69 @@ export class TaskService {
}
async updateTask(id: string, input: UpdateTaskInput): Promise<Task | null> {
// 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<TaskTelemetryEvent>({
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<TaskTelemetryEvent>({
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<Task> {
async getRunningTimerTask(): Promise<Task | null> {
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<Task>;
const updated = (await this.updateTask(taskId, { timeTracking })) as Task;
return { task: updated, stoppedTaskId };
}
/**

112
web/src/components/task/TimeTrackingSection.tsx
View file

@ -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<boolean | null>(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 (
<div className="space-y-4">
<div className="flex items-center justify-between">
@ -143,26 +155,34 @@ export function TimeTrackingSection({ task }: TimeTrackingSectionProps) {
{/* Timer controls */}
<div className="flex items-center justify-between">
<div className="flex items-center gap-3">
<Button
variant={isRunning ? 'destructive' : 'default'}
size="sm"
onClick={handleStartStop}
disabled={startTimer.isPending || stopTimer.isPending}
>
{startTimer.isPending || stopTimer.isPending ? (
<Loader2 className="h-4 w-4 animate-spin" />
) : isRunning ? (
<>
<Square className="h-4 w-4 mr-2" />
Stop
</>
) : (
<>
<Play className="h-4 w-4 mr-2" />
Start
</>
)}
</Button>
{/* Only show Start/Stop when this task's timer is running OR no other timer is active */}
{isRunning ? (
<Button variant="destructive" size="sm" onClick={handleStartStop} disabled={isBusy}>
{isBusy ? (
<Loader2 className="h-4 w-4 animate-spin" />
) : (
<>
<Square className="h-4 w-4 mr-2" />
Stop
</>
)}
</Button>
) : !otherRunningTask ? (
<Button variant="default" size="sm" onClick={handleStartStop} disabled={isBusy}>
{isBusy ? (
<Loader2 className="h-4 w-4 animate-spin" />
) : (
<>
<Play className="h-4 w-4 mr-2" />
Start
</>
)}
</Button>
) : (
<span className="text-xs text-muted-foreground italic">
Timer active on another task
</span>
)}
{isRunning && activeEntry && (
<div className="flex items-center gap-2">
@ -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'
)}
>
<div className="flex-1 min-w-0">
<div className="flex items-center gap-2">
{entry.id === task.timeTracking?.activeEntryId ? (
{entry.id === timerTask.timeTracking?.activeEntryId ? (
<Timer className="h-3 w-3 text-green-500 animate-pulse flex-shrink-0" />
) : (
<Clock className="h-3 w-3 text-muted-foreground flex-shrink-0" />
@ -272,7 +292,7 @@ export function TimeTrackingSection({ task }: TimeTrackingSectionProps) {
: formatEntryTime(entry)}
</div>
</div>
{entry.id !== task.timeTracking?.activeEntryId && (
{entry.id !== timerTask.timeTracking?.activeEntryId && (
<Button
variant="ghost"
size="sm"