fix: persist project/task changes when editing a time entry #209
+91
−49
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| 'use client'; | ||
|
|
||
| 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,39 +138,51 @@ export function TimeEntryModal({ isOpen, onClose, date, entry, weekStart }: Time | |
| [customerOptions] | ||
| ); | ||
|
|
||
| // Reset form only when the modal opens or the entry/date being edited | ||
| // 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<string | null>(null); | ||
| useEffect(() => { | ||
| const resetKey = isOpen ? `${entry?.id ?? 'new'}|${date ?? ''}` : null; | ||
| if (!isOpen) { | ||
| lastResetKeyRef.current = null; | ||
| return; | ||
| } | ||
| 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, | ||
|
|
@@ -239,23 +251,53 @@ export function TimeEntryModal({ isOpen, onClose, date, entry, weekStart }: Time | |
| setIsSubmitting(true); | ||
| try { | ||
| 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 | ||
| // 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 addEntry({ | ||
| projectId: jobNo, | ||
| taskId: jobTaskNo, | ||
| userId, | ||
| date: selectedDate, | ||
| hours: totalHours, | ||
| notes, | ||
| isBillable: task?.isBillable ?? true, | ||
| isRunning: false, | ||
| }); | ||
|
Comment on lines
+259
to
+269
Contributor
Author
There was a problem hiding this comment. Fair point — reordered to create the replacement entry first and only delete the original after the create succeeds. A mid-flight BC failure now leaves the original hours/notes intact; the worst case is a transient duplicate entry if the delete fails after the create, which is recoverable from the UI. Fixed in 2f15dcb. |
||
| // 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 { | ||
|
Comment on lines
+259
to
+282
Contributor
Author
There was a problem hiding this comment. Added a try/catch around the post-add delete in 9b3bc0b. If the delete fails after the create succeeds, the user now sees a specific toast ("New entry saved, but the original could not be removed. Please delete it manually.") instead of a generic save failure, so they know exactly what to clean up. A full automated rollback could itself fail, so surfacing the partial-success state was the safer call. |
||
| // 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<TimeEntry> = {}; | ||
| 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 | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right — the comment was misleading. Updated in 9b3bc0b to describe what the guard actually does: the resetKey ref intentionally suppresses background dep changes (including a projects refresh) so an in-progress edit isn't clobbered. Deps stay listed to keep exhaustive-deps happy; the guard makes them no-ops.