From 63ae6e8dc39a765bd9de8fecd56bd2668fc4f591 Mon Sep 17 00:00:00 2001 From: EdiWeeks <121399761+EdiWeeks@users.noreply.github.com> Date: Fri, 22 May 2026 08:22:30 +0100 Subject: [PATCH 1/4] fix: persist project/task changes when editing a time entry The edit modal's reset effect re-ran whenever selectedProject/selectedTask changed in the store, which happened mid-edit and clobbered the user's new selection. Submitting also only forwarded hours/notes, so even visual edits to project/task were silently dropped. A BC line is bound to one project+task, so a project/task change now replaces the entry (delete + add) instead of attempting an in-place patch. Fixes #208. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/components/timesheet/TimeEntryModal.tsx | 61 ++++++++++++++------- 1 file changed, 42 insertions(+), 19 deletions(-) diff --git a/src/components/timesheet/TimeEntryModal.tsx b/src/components/timesheet/TimeEntryModal.tsx index 6264886..02fb666 100644 --- a/src/components/timesheet/TimeEntryModal.tsx +++ b/src/components/timesheet/TimeEntryModal.tsx @@ -138,7 +138,12 @@ export function TimeEntryModal({ isOpen, onClose, date, entry, weekStart }: Time [customerOptions] ); - // Reset form when modal opens + // Reset form when modal opens or a different entry is loaded. + // Intentionally omits selectedProject/selectedTask/projects/findMatchingCustomerOption + // from deps: handleProjectChange/handleTaskChange call selectProject/selectTask, + // which would otherwise re-run this effect and clobber the user's new selection + // with the original entry values (#208). + // eslint-disable-next-line react-hooks/exhaustive-deps useEffect(() => { if (isOpen) { if (entry) { @@ -170,7 +175,7 @@ export function TimeEntryModal({ isOpen, onClose, date, entry, weekStart }: Time setNotes(''); } } - }, [isOpen, entry, date, selectedProject, selectedTask, projects, findMatchingCustomerOption]); + }, [isOpen, entry, date]); const projectOptions: SelectOption[] = filteredProjects.map((p) => ({ value: p.id, @@ -239,23 +244,41 @@ export function TimeEntryModal({ isOpen, onClose, date, entry, weekStart }: Time setIsSubmitting(true); try { if (entry) { - // If date changed, move the entry first so subsequent updates target the new detail - let targetEntryId = entry.id; - if (selectedDate !== entry.date) { - await moveEntryDate(entry.id, selectedDate); - // Composite ID format is `{lineId}_{date}` — recompute for follow-up update - const lineId = entry.bcTimeSheetLineId || entry.id.replace(/_\d{4}-\d{2}-\d{2}$/, ''); - targetEntryId = `${lineId}_${selectedDate}`; - } - // Only forward fields the user actually changed. If only the date - // changed, skipping updateEntry preserves any same-line merge that - // moveEntryDate produced — otherwise the form's hours would clobber - // the merged total on the target date. - const updates: Partial = {}; - if (totalHours !== entry.hours) updates.hours = totalHours; - if (notes !== (entry.notes || '')) updates.notes = notes; - if (Object.keys(updates).length > 0) { - await updateEntry(targetEntryId, updates); + // A BC timesheet line is bound to a single project+task, so a project + // or task change can't be patched in place — replace the entry by + // deleting the original and creating a new one on the chosen date. + if (jobNo !== entry.projectId || jobTaskNo !== entry.taskId) { + await deleteEntry(entry.id); + await addEntry({ + projectId: jobNo, + taskId: jobTaskNo, + userId, + date: selectedDate, + hours: totalHours, + notes, + isBillable: task?.isBillable ?? true, + isRunning: false, + }); + } else { + // If date changed, move the entry first so subsequent updates target the new detail + let targetEntryId = entry.id; + if (selectedDate !== entry.date) { + await moveEntryDate(entry.id, selectedDate); + // Composite ID format is `{lineId}_{date}` — recompute for follow-up update + const lineId = + entry.bcTimeSheetLineId || entry.id.replace(/_\d{4}-\d{2}-\d{2}$/, ''); + targetEntryId = `${lineId}_${selectedDate}`; + } + // Only forward fields the user actually changed. If only the date + // changed, skipping updateEntry preserves any same-line merge that + // moveEntryDate produced — otherwise the form's hours would clobber + // the merged total on the target date. + const updates: Partial = {}; + if (totalHours !== entry.hours) updates.hours = totalHours; + if (notes !== (entry.notes || '')) updates.notes = notes; + if (Object.keys(updates).length > 0) { + await updateEntry(targetEntryId, updates); + } } } else { // Create new entry From d85e1fb88f9ee92af086bd0d77e40e088e8fa366 Mon Sep 17 00:00:00 2001 From: EdiWeeks <121399761+EdiWeeks@users.noreply.github.com> Date: Fri, 22 May 2026 08:30:50 +0100 Subject: [PATCH 2/4] style: apply prettier Co-Authored-By: Claude Opus 4.7 (1M context) --- src/components/timesheet/TimeEntryModal.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/components/timesheet/TimeEntryModal.tsx b/src/components/timesheet/TimeEntryModal.tsx index 02fb666..16903f9 100644 --- a/src/components/timesheet/TimeEntryModal.tsx +++ b/src/components/timesheet/TimeEntryModal.tsx @@ -265,8 +265,7 @@ export function TimeEntryModal({ isOpen, onClose, date, entry, weekStart }: Time if (selectedDate !== entry.date) { await moveEntryDate(entry.id, selectedDate); // Composite ID format is `{lineId}_{date}` — recompute for follow-up update - const lineId = - entry.bcTimeSheetLineId || entry.id.replace(/_\d{4}-\d{2}-\d{2}$/, ''); + const lineId = entry.bcTimeSheetLineId || entry.id.replace(/_\d{4}-\d{2}-\d{2}$/, ''); targetEntryId = `${lineId}_${selectedDate}`; } // Only forward fields the user actually changed. If only the date From 2f15dcb25750e923fe586ffd498ece29ea31c742 Mon Sep 17 00:00:00 2001 From: EdiWeeks <121399761+EdiWeeks@users.noreply.github.com> Date: Fri, 22 May 2026 08:34:07 +0100 Subject: [PATCH 3/4] refactor: address copilot review on edit modal - Replace eslint-disable on the reset effect with a resetKey ref so the body still re-runs on background projects refreshes, just not on incidental dep churn from selectProject/selectTask. - Create the replacement entry before deleting the original when project or task changes, so a BC failure mid-flight no longer risks losing the user's original hours and notes. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/components/timesheet/TimeEntryModal.tsx | 87 ++++++++++++--------- 1 file changed, 48 insertions(+), 39 deletions(-) diff --git a/src/components/timesheet/TimeEntryModal.tsx b/src/components/timesheet/TimeEntryModal.tsx index 16903f9..bb83bb5 100644 --- a/src/components/timesheet/TimeEntryModal.tsx +++ b/src/components/timesheet/TimeEntryModal.tsx @@ -1,6 +1,6 @@ 'use client'; -import { useState, useEffect, useMemo, useCallback } from 'react'; +import { useState, useEffect, useMemo, useCallback, useRef } from 'react'; import toast from 'react-hot-toast'; import { TrashIcon, ExclamationTriangleIcon } from '@heroicons/react/24/outline'; import { Modal, Button, Input, Select } from '@/components/ui'; @@ -138,44 +138,51 @@ export function TimeEntryModal({ isOpen, onClose, date, entry, weekStart }: Time [customerOptions] ); - // Reset form when modal opens or a different entry is loaded. - // Intentionally omits selectedProject/selectedTask/projects/findMatchingCustomerOption - // from deps: handleProjectChange/handleTaskChange call selectProject/selectTask, - // which would otherwise re-run this effect and clobber the user's new selection - // with the original entry values (#208). - // eslint-disable-next-line react-hooks/exhaustive-deps + // Reset form only when the modal opens or the entry/date being edited + // changes. The effect still depends on projects/findMatchingCustomerOption + // so a background refresh produces consistent state, but a resetKey guard + // skips the body on incidental dep changes (e.g. handleProjectChange firing + // selectProject mid-edit) so the user's in-progress selection is not + // clobbered with the original entry values (#208). + const lastResetKeyRef = useRef(null); useEffect(() => { - if (isOpen) { - if (entry) { - // Editing existing entry - entry.projectId is a job code (e.g., "PR00030"), not a GUID - const project = projects.find((p) => p.code === entry.projectId); - // Use matching customer option value to ensure Select works correctly - const matchedCustomer = findMatchingCustomerOption(project?.customerName); - setCustomerId(matchedCustomer); - // Use project.id (GUID) for form state since dropdown options use GUIDs - setProjectId(project?.id || ''); - // Find task by code and use its id - const task = project?.tasks.find((t) => t.code === entry.taskId); - setTaskId(task?.id || ''); - setSelectedDate(entry.date); - const h = Math.floor(entry.hours); - const m = Math.round((entry.hours - h) * 60); - setHours(h.toString()); - setMinutes(m.toString()); - setNotes(entry.notes || ''); - } else { - // New entry - use matching customer option value - const matchedCustomer = findMatchingCustomerOption(selectedProject?.customerName); - setCustomerId(matchedCustomer); - setProjectId(selectedProject?.id || ''); - setTaskId(selectedTask?.id || ''); - setSelectedDate(date || ''); - setHours(''); - setMinutes(''); - setNotes(''); - } + const resetKey = isOpen ? `${entry?.id ?? 'new'}|${date ?? ''}` : null; + if (!isOpen) { + lastResetKeyRef.current = null; + return; } - }, [isOpen, entry, date]); + if (lastResetKeyRef.current === resetKey) return; + lastResetKeyRef.current = resetKey; + + if (entry) { + // Editing existing entry - entry.projectId is a job code (e.g., "PR00030"), not a GUID + const project = projects.find((p) => p.code === entry.projectId); + // Use matching customer option value to ensure Select works correctly + const matchedCustomer = findMatchingCustomerOption(project?.customerName); + setCustomerId(matchedCustomer); + // Use project.id (GUID) for form state since dropdown options use GUIDs + setProjectId(project?.id || ''); + // Find task by code and use its id + const task = project?.tasks.find((t) => t.code === entry.taskId); + setTaskId(task?.id || ''); + setSelectedDate(entry.date); + const h = Math.floor(entry.hours); + const m = Math.round((entry.hours - h) * 60); + setHours(h.toString()); + setMinutes(m.toString()); + setNotes(entry.notes || ''); + } else { + // New entry - use matching customer option value + const matchedCustomer = findMatchingCustomerOption(selectedProject?.customerName); + setCustomerId(matchedCustomer); + setProjectId(selectedProject?.id || ''); + setTaskId(selectedTask?.id || ''); + setSelectedDate(date || ''); + setHours(''); + setMinutes(''); + setNotes(''); + } + }, [isOpen, entry, date, projects, findMatchingCustomerOption, selectedProject, selectedTask]); const projectOptions: SelectOption[] = filteredProjects.map((p) => ({ value: p.id, @@ -246,9 +253,10 @@ export function TimeEntryModal({ isOpen, onClose, date, entry, weekStart }: Time if (entry) { // A BC timesheet line is bound to a single project+task, so a project // or task change can't be patched in place — replace the entry by - // deleting the original and creating a new one on the chosen date. + // creating the new one first and then deleting the original. Doing + // the create before the delete means a BC failure mid-flight leaves + // the original hours intact instead of silently losing them. if (jobNo !== entry.projectId || jobTaskNo !== entry.taskId) { - await deleteEntry(entry.id); await addEntry({ projectId: jobNo, taskId: jobTaskNo, @@ -259,6 +267,7 @@ export function TimeEntryModal({ isOpen, onClose, date, entry, weekStart }: Time isBillable: task?.isBillable ?? true, isRunning: false, }); + await deleteEntry(entry.id); } else { // If date changed, move the entry first so subsequent updates target the new detail let targetEntryId = entry.id; From 9b3bc0b23a201aa70a0121f953d2f5e84381d0e4 Mon Sep 17 00:00:00 2001 From: EdiWeeks <121399761+EdiWeeks@users.noreply.github.com> Date: Fri, 22 May 2026 08:49:04 +0100 Subject: [PATCH 4/4] refactor: address second copilot pass - Surface a specific toast and close the modal when the post-add delete fails, so the user knows the original line still exists and needs to be removed manually rather than seeing a generic save error. - Correct the reset-effect comment to match the actual guard behavior: the resetKey ref intentionally skips background dep changes (including projects refreshes) to protect in-progress edits, not to forward them. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/components/timesheet/TimeEntryModal.tsx | 23 +++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/components/timesheet/TimeEntryModal.tsx b/src/components/timesheet/TimeEntryModal.tsx index bb83bb5..38ae0fd 100644 --- a/src/components/timesheet/TimeEntryModal.tsx +++ b/src/components/timesheet/TimeEntryModal.tsx @@ -139,11 +139,11 @@ export function TimeEntryModal({ isOpen, onClose, date, entry, weekStart }: Time ); // Reset form only when the modal opens or the entry/date being edited - // changes. The effect still depends on projects/findMatchingCustomerOption - // so a background refresh produces consistent state, but a resetKey guard - // skips the body on incidental dep changes (e.g. handleProjectChange firing - // selectProject mid-edit) so the user's in-progress selection is not - // clobbered with the original entry values (#208). + // changes. A resetKey ref gates the body to the open-transition and entry + // identity, so unrelated dep changes — selectProject/selectTask firing + // mid-edit, or a background projects refresh — don't clobber the user's + // in-progress selection with the original entry values (#208). Deps are + // still listed honestly for exhaustive-deps; the guard makes them no-ops. const lastResetKeyRef = useRef(null); useEffect(() => { const resetKey = isOpen ? `${entry?.id ?? 'new'}|${date ?? ''}` : null; @@ -267,7 +267,18 @@ export function TimeEntryModal({ isOpen, onClose, date, entry, weekStart }: Time isBillable: task?.isBillable ?? true, isRunning: false, }); - await deleteEntry(entry.id); + // The add succeeded; if the delete fails the user briefly has both + // the old and new entries. Surface a specific message so they know + // exactly what to clean up rather than a generic save failure. + try { + await deleteEntry(entry.id); + } catch { + toast.error( + 'New entry saved, but the original could not be removed. Please delete it manually.' + ); + onClose(); + return; + } } else { // If date changed, move the entry first so subsequent updates target the new detail let targetEntryId = entry.id;