Change property to "" does not get saved in patches #45

New issue

Closed

opened 2026-03-29 16:38:26 +00:00 by maxtkc · 0 comments

maxtkc commented

2026-03-29 16:38:26 +00:00

Owner

Summary

schedule-controller.ts captures the before value for trip property edits using getFileDataSync('trips') (missing .txt suffix). Every other caller in the codebase uses 'trips.txt'. The wrong key returns an empty array, so currentTrip is always undefined and before[field] is always null. When the user clears a field to "", processedValue converts to null, producing before = null and after = null — recordUpdate detects no change and returns early without writing a patch. The notification fires unconditionally regardless, so the user sees a success message but nothing is recorded.

This bug also silently corrupts undo for all trip property edits, not just the empty-string case: the inverse patch stores null as the original value rather than the real prior value, so undoing any trip property edit incorrectly nulls it out instead of restoring the previous string.

The fix is a one-line change plus suppressing the spurious notification when nothing was actually changed.

Relevant context

src/modules/schedule-controller.ts:492 — the broken getFileDataSync('trips') call inside updateTripProperty. The before object is built here immediately before calling patchUpdate.
src/modules/patch-manager.ts:167–192 — recordUpdate compares before[key] !== after[key] and returns early (line 181) if nothing changed. No exception is thrown — callers cannot tell whether a patch was created.
src/utils/patch-utils.ts:42–45 — patchUpdate always calls db.updateRow first, then calls pm.recordUpdate. Because db.updateRow runs unconditionally, the DB value IS changed even when the patch is not recorded.
src/modules/timetable-renderer.ts:341,357,373 — the inline onchange handlers that invoke updateTripProperty with this.value.
src/utils/form-patch-bridge.ts:59–62 — the correct pattern: captures before as String(row?.[field] ?? ''), compares strings, and early-returns if unchanged before calling recordUpdate.
All other getFileDataSync callers (gtfs-relationships.ts, timetable-database.ts, info-display.ts) correctly use the .txt suffix.

Phase 1 — Fix the before-value capture and notification

Goal: correct the root cause so before reflects the actual stored value, and suppress the spurious notification when the value is unchanged.

In src/modules/schedule-controller.ts:492, change getFileDataSync('trips') → getFileDataSync('trips.txt')
After capturing before and computing processedValue, add an early-return guard: if the processed new value strictly equals the stored before value, show no notification and return (mirrors the pattern in form-patch-bridge.ts:61)
Verify the early-return covers both the empty-string-to-null conversion (text/number fields) and the enum fields (direction_id, wheelchair_accessible, bikes_allowed): in both branches, compare processedValue against currentTrip?.[field] ?? null before proceeding

Implemented in commit 214ec73. The guard uses processedValue === storedValue (strict equality), which correctly handles both numeric enum fields and null/string comparisons. The stored value is captured as currentTrip?.[field] ?? null matching the same default used for before.

Phase 2 — Fix stop time edits not appearing in Changes

Goal: make arrival/departure time edits in the timetable produce visible patches in the Changes panel, and correctly record the inverse so undo restores the prior value.

Root cause: updateArrivalDepartureTime and updateLinkedTime both call rebuildStopTimesFromTable followed by refreshCurrentTimetable, then try to find the updated row by reading afterInput.dataset.stopSequence from the re-rendered DOM. dataset.stopSequence is always a string (e.g. "2"), but rebuildStopTimesFromTable stores stop_sequence as a JavaScript number (index + 1). The virtual table's query filter uses strict equality (r['stop_sequence'] === v), so 1 === "1" is false. getStopTime returns null, the if (beforeRow && afterStopTime && this.patchManager) guard is never entered, and no patch is recorded.

Relevant context:

src/modules/timetable-database.ts:422 — getStopTime: when stop_sequence is provided, it passes it directly into queryRows without coercion; the string-vs-number mismatch silently returns no results.
src/modules/timetable-database.ts:588 — rebuildStopTimesFromTable: assigns stop_sequence: index + 1 (number) on the new rows.
src/modules/schedule-controller.ts:488–509 — updateArrivalDepartureTime: reads afterInput.dataset.stopSequence (string) and passes it to getStopTime.
src/modules/schedule-controller.ts:322–350 — updateLinkedTime (set branch): same pattern.
src/modules/gtfs-parser.ts:246–252 — virtual table query: uses strict === comparison for filter values not covered by a fieldMap (stop_sequence has no fieldMap, only trip_id and stop_id do).

Steps:

In src/modules/timetable-database.ts, in getStopTime (line 435), when stop_sequence is provided, coerce it to a number before passing to queryRows: stop_sequence: Number(stop_sequence). This matches the type stored by rebuildStopTimesFromTable.
Verify updateArrivalDepartureTime and updateLinkedTime now produce entries in the Changes panel after editing a time.
Verify that undo correctly restores the previous time value.

Implemented in commit 9e88350. One-line fix: stop_sequence: Number(stop_sequence) in getStopTime. The strict-equality mismatch between dataset strings and stored numbers was the sole reason patches were never recorded for stop time edits.

Phase 3 — Fix duplicate notifications on arrival/departure time edits

Goal: stop the user from seeing two notifications per time edit (one "Time updated" toast from timetable-database.ts, one from the patchManager 'change' event via index.ts).

Root cause: timetable-database.ts:updateStopTimeInDatabase fires notifications.showSuccess("Time updated to …") (line 295) unconditionally, and updateLinkedTimes fires notifications.showSuccess("Linked time updated to …") (line 407). After these calls return, schedule-controller.ts calls patchManager.recordUpdate(...), which emits 'change', and index.ts:171 fires a second notification (notifications.showInfo(humanLabel(r?.patch), ...)). The result is two toasts per edit.

Relevant context:

src/modules/timetable-database.ts:295 — notifications.showSuccess at end of updateStopTimeInDatabase.
src/modules/timetable-database.ts:407 — notifications.showSuccess at end of updateLinkedTimes.
src/index.ts:169–171 — the patchManager.on('change', ...) handler that fires notifications.showInfo(humanLabel(r?.patch), ...) for every recorded patch. This is the canonical success notification for all patch-backed edits.

Steps:

Remove the notifications.showSuccess(...) call from timetable-database.ts:updateStopTimeInDatabase (line 295). Success feedback is already provided by the patchManager 'change' event handler in index.ts.
Remove the notifications.showSuccess(...) call from timetable-database.ts:updateLinkedTimes (line 407) for the same reason.

Implemented in commit ea41124. Removed both showSuccess calls; the canonical patchManager 'change' handler in index.ts now provides the sole notification for time edits.

Phase 4 — Record patch when toggling from unlinked to linked

Goal: when the user clicks the link button to switch from separate arrival/departure inputs to a single linked input (setting departure = arrival), record an undo-able patch for the departure time change.

Root cause: swapToLinkedInput (schedule-controller.ts:597) calls this.database.updateLinkedTimes(trip_id, stop_id, primaryTime) when arrival_time !== departure_time, but never calls patchManager.recordUpdate(). The DB write happens with no patch entry, so the change is invisible in the Changes panel and undo cannot restore the previous departure time.

Relevant context:

src/modules/schedule-controller.ts:597–641 — swapToLinkedInput: captures stopTime from DB (line 612), determines primaryTime (line 618), calls updateLinkedTimes (line 635), but makes no patch call.
src/modules/schedule-controller.ts:333–350 — updateLinkedTime (set branch): the correct pattern — captures beforeRow, calls updateLinkedTimes, then calls patchManager.recordUpdate with { arrival_time, departure_time } before/after.
Patch should only be recorded if departure_time !== primaryTime (i.e. the DB was actually changed). If arrival and departure were already equal, there is nothing to record.

Steps:

In swapToLinkedInput, before calling updateLinkedTimes, capture beforeDepartureTime = stopTime?.departure_time and beforeArrivalTime = stopTime?.arrival_time.
After calling updateLinkedTimes, if primaryTime is set and this.patchManager is available and the stored times actually changed (i.e. beforeArrivalTime !== primaryTime || beforeDepartureTime !== primaryTime), look up the updated stop time and call patchManager.recordUpdate with the before/after { arrival_time, departure_time } — mirroring the pattern in updateLinkedTime set branch (lines 333–350).

Implemented in commit fa80330. The existing arrival_time and departure_time variables captured at lines 614–615 serve as the before-state. The patch guard uses arrival_time !== primaryTime || departure_time !== primaryTime to skip no-op toggles (when times were already equal).

Original Issue

At least for trip shape and I think other properties

## Summary `schedule-controller.ts` captures the `before` value for trip property edits using `getFileDataSync('trips')` (missing `.txt` suffix). Every other caller in the codebase uses `'trips.txt'`. The wrong key returns an empty array, so `currentTrip` is always `undefined` and `before[field]` is always `null`. When the user clears a field to `""`, `processedValue` converts to `null`, producing `before = null` and `after = null` — `recordUpdate` detects no change and returns early without writing a patch. The notification fires unconditionally regardless, so the user sees a success message but nothing is recorded. This bug also silently corrupts undo for **all** trip property edits, not just the empty-string case: the inverse patch stores `null` as the original value rather than the real prior value, so undoing any trip property edit incorrectly nulls it out instead of restoring the previous string. The fix is a one-line change plus suppressing the spurious notification when nothing was actually changed. ## Relevant context - **`src/modules/schedule-controller.ts:492`** — the broken `getFileDataSync('trips')` call inside `updateTripProperty`. The `before` object is built here immediately before calling `patchUpdate`. - **`src/modules/patch-manager.ts:167–192`** — `recordUpdate` compares `before[key] !== after[key]` and returns early (line 181) if nothing changed. No exception is thrown — callers cannot tell whether a patch was created. - **`src/utils/patch-utils.ts:42–45`** — `patchUpdate` always calls `db.updateRow` first, then calls `pm.recordUpdate`. Because `db.updateRow` runs unconditionally, the DB value IS changed even when the patch is not recorded. - **`src/modules/timetable-renderer.ts:341,357,373`** — the inline `onchange` handlers that invoke `updateTripProperty` with `this.value`. - **`src/utils/form-patch-bridge.ts:59–62`** — the correct pattern: captures `before` as `String(row?.[field] ?? '')`, compares strings, and early-returns if unchanged before calling `recordUpdate`. - All other `getFileDataSync` callers (gtfs-relationships.ts, timetable-database.ts, info-display.ts) correctly use the `.txt` suffix. ## Phase 1 — Fix the before-value capture and notification Goal: correct the root cause so `before` reflects the actual stored value, and suppress the spurious notification when the value is unchanged. - [x] In `src/modules/schedule-controller.ts:492`, change `getFileDataSync('trips')` → `getFileDataSync('trips.txt')` - [x] After capturing `before` and computing `processedValue`, add an early-return guard: if the processed new value strictly equals the stored before value, show no notification and return (mirrors the pattern in `form-patch-bridge.ts:61`) - [x] Verify the early-return covers both the empty-string-to-null conversion (text/number fields) and the enum fields (`direction_id`, `wheelchair_accessible`, `bikes_allowed`): in both branches, compare `processedValue` against `currentTrip?.[field] ?? null` before proceeding Implemented in commit `214ec73`. The guard uses `processedValue === storedValue` (strict equality), which correctly handles both numeric enum fields and null/string comparisons. The stored value is captured as `currentTrip?.[field] ?? null` matching the same default used for `before`. ## Phase 2 — Fix stop time edits not appearing in Changes Goal: make arrival/departure time edits in the timetable produce visible patches in the Changes panel, and correctly record the inverse so undo restores the prior value. **Root cause**: `updateArrivalDepartureTime` and `updateLinkedTime` both call `rebuildStopTimesFromTable` followed by `refreshCurrentTimetable`, then try to find the updated row by reading `afterInput.dataset.stopSequence` from the re-rendered DOM. `dataset.stopSequence` is always a string (e.g. `"2"`), but `rebuildStopTimesFromTable` stores `stop_sequence` as a JavaScript number (`index + 1`). The virtual table's query filter uses strict equality (`r['stop_sequence'] === v`), so `1 === "1"` is `false`. `getStopTime` returns `null`, the `if (beforeRow && afterStopTime && this.patchManager)` guard is never entered, and no patch is recorded. **Relevant context**: - **`src/modules/timetable-database.ts:422`** — `getStopTime`: when `stop_sequence` is provided, it passes it directly into `queryRows` without coercion; the string-vs-number mismatch silently returns no results. - **`src/modules/timetable-database.ts:588`** — `rebuildStopTimesFromTable`: assigns `stop_sequence: index + 1` (number) on the new rows. - **`src/modules/schedule-controller.ts:488–509`** — `updateArrivalDepartureTime`: reads `afterInput.dataset.stopSequence` (string) and passes it to `getStopTime`. - **`src/modules/schedule-controller.ts:322–350`** — `updateLinkedTime` (set branch): same pattern. - **`src/modules/gtfs-parser.ts:246–252`** — virtual table `query`: uses strict `===` comparison for filter values not covered by a fieldMap (stop_sequence has no fieldMap, only `trip_id` and `stop_id` do). **Steps**: - [x] In `src/modules/timetable-database.ts`, in `getStopTime` (line 435), when `stop_sequence` is provided, coerce it to a number before passing to `queryRows`: `stop_sequence: Number(stop_sequence)`. This matches the type stored by `rebuildStopTimesFromTable`. - [x] Verify `updateArrivalDepartureTime` and `updateLinkedTime` now produce entries in the Changes panel after editing a time. - [x] Verify that undo correctly restores the previous time value. Implemented in commit `9e88350`. One-line fix: `stop_sequence: Number(stop_sequence)` in `getStopTime`. The strict-equality mismatch between dataset strings and stored numbers was the sole reason patches were never recorded for stop time edits. ## Phase 3 — Fix duplicate notifications on arrival/departure time edits Goal: stop the user from seeing two notifications per time edit (one "Time updated" toast from `timetable-database.ts`, one from the `patchManager` `'change'` event via `index.ts`). **Root cause**: `timetable-database.ts:updateStopTimeInDatabase` fires `notifications.showSuccess("Time updated to …")` (line 295) unconditionally, and `updateLinkedTimes` fires `notifications.showSuccess("Linked time updated to …")` (line 407). After these calls return, `schedule-controller.ts` calls `patchManager.recordUpdate(...)`, which emits `'change'`, and `index.ts:171` fires a second notification (`notifications.showInfo(humanLabel(r?.patch), ...)`). The result is two toasts per edit. **Relevant context**: - **`src/modules/timetable-database.ts:295`** — `notifications.showSuccess` at end of `updateStopTimeInDatabase`. - **`src/modules/timetable-database.ts:407`** — `notifications.showSuccess` at end of `updateLinkedTimes`. - **`src/index.ts:169–171`** — the `patchManager.on('change', ...)` handler that fires `notifications.showInfo(humanLabel(r?.patch), ...)` for every recorded patch. This is the canonical success notification for all patch-backed edits. **Steps**: - [x] Remove the `notifications.showSuccess(...)` call from `timetable-database.ts:updateStopTimeInDatabase` (line 295). Success feedback is already provided by the `patchManager` `'change'` event handler in `index.ts`. - [x] Remove the `notifications.showSuccess(...)` call from `timetable-database.ts:updateLinkedTimes` (line 407) for the same reason. Implemented in commit `ea41124`. Removed both `showSuccess` calls; the canonical `patchManager` `'change'` handler in `index.ts` now provides the sole notification for time edits. ## Phase 4 — Record patch when toggling from unlinked to linked Goal: when the user clicks the link button to switch from separate arrival/departure inputs to a single linked input (setting departure = arrival), record an undo-able patch for the departure time change. **Root cause**: `swapToLinkedInput` (schedule-controller.ts:597) calls `this.database.updateLinkedTimes(trip_id, stop_id, primaryTime)` when `arrival_time !== departure_time`, but never calls `patchManager.recordUpdate()`. The DB write happens with no patch entry, so the change is invisible in the Changes panel and undo cannot restore the previous departure time. **Relevant context**: - **`src/modules/schedule-controller.ts:597–641`** — `swapToLinkedInput`: captures `stopTime` from DB (line 612), determines `primaryTime` (line 618), calls `updateLinkedTimes` (line 635), but makes no patch call. - **`src/modules/schedule-controller.ts:333–350`** — `updateLinkedTime` (set branch): the correct pattern — captures `beforeRow`, calls `updateLinkedTimes`, then calls `patchManager.recordUpdate` with `{ arrival_time, departure_time }` before/after. - Patch should only be recorded if `departure_time !== primaryTime` (i.e. the DB was actually changed). If arrival and departure were already equal, there is nothing to record. **Steps**: - [x] In `swapToLinkedInput`, before calling `updateLinkedTimes`, capture `beforeDepartureTime = stopTime?.departure_time` and `beforeArrivalTime = stopTime?.arrival_time`. - [x] After calling `updateLinkedTimes`, if `primaryTime` is set and `this.patchManager` is available and the stored times actually changed (i.e. `beforeArrivalTime !== primaryTime || beforeDepartureTime !== primaryTime`), look up the updated stop time and call `patchManager.recordUpdate` with the before/after `{ arrival_time, departure_time }` — mirroring the pattern in `updateLinkedTime` set branch (lines 333–350). Implemented in commit `fa80330`. The existing `arrival_time` and `departure_time` variables captured at lines 614–615 serve as the before-state. The patch guard uses `arrival_time !== primaryTime || departure_time !== primaryTime` to skip no-op toggles (when times were already equal). --- ## Original Issue At least for trip shape and I think other properties