fix: persist project/task changes when editing a time entry#209
fix: persist project/task changes when editing a time entry#209EdiWeeks wants to merge 4 commits into
Conversation
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes an issue in the timesheet time-entry edit modal where changing project/task selections would revert and where project/task updates were not persisted to Business Central. The PR also aligns edit behavior with BC’s constraint that a timesheet line is bound to a single project+task by treating project/task changes as a replacement operation.
Changes:
- Prevent the modal “reset form” effect from re-running on project/task selection changes (fixes selection snapping back).
- Persist project/task edits by replacing the original entry (delete + create) when project/task changes; keep in-place update behavior for hours/notes/date-only edits.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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 |
There was a problem hiding this comment.
Good catch — replaced the eslint-disable with a resetKey ref. The effect now keeps projects and findMatchingCustomerOption in its dep list (so a background projects refresh still produces consistent state), and a ${entry?.id}|${date} resetKey gates the body so incidental dep changes from selectProject/selectTask don't re-fire the reset. Fixed in 2f15dcb.
| 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, | ||
| }); |
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.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- 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) <noreply@anthropic.com>
| 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, | ||
| }); | ||
| await deleteEntry(entry.id); | ||
| } else { |
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.
| // 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<string | null>(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, selectedProject, selectedTask, projects, findMatchingCustomerOption]); | ||
| if (lastResetKeyRef.current === resetKey) return; | ||
| lastResetKeyRef.current = resetKey; |
There was a problem hiding this comment.
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.
- 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) <noreply@anthropic.com>
Summary
selectedProject/selectedTask(and friends) in deps, so picking a new project mid-edit firedselectProject→ re-ran the effect → snapped the form back to the original entry values.handleSubmitonly forwardedhours/noteson update — project and task changes were silently dropped even when the UI did show them.Fixes #208
Test plan
🤖 Generated with Claude Code