gtfs.zone/coloring-book

Fork 0

Support deleting objects #48

New issue

Closed

opened 2026-03-29 22:26:58 +00:00 by maxtkc · 1 comment

maxtkc commented

2026-03-29 22:26:58 +00:00

Owner

Summary

Add the ability to delete GTFS objects, starting with stops. The patch system already fully supports delete operations — patchManager.recordDelete(), undo/redo replay, and the history UI red badge are all in place. The work is purely in wiring a delete button to the stop view UI, implementing a FK-aware confirmation modal (showing affected stop_times with an option to cascade-delete them all), and navigating home after deletion. The cascade pattern is borrowed from the existing removeException flow in service-days-controller.ts.

Key tradeoff: Instead of adding mutation logic to StopViewController (which is a view-only component), we use a callback pattern (onDeleteStop) so all DB writes stay in PageContentRenderer. This preserves the architectural boundary.

Relevant Context

Patch system: src/modules/patch-manager.ts — recordDelete(table, id, record) at line 194. Already full undo/redo support (inverse re-inserts from patch.inverse.record).
History UI: src/modules/history-controller.ts — already renders delete patches with red badge-error. No changes needed.
Stop view: src/modules/stop-view-controller.ts — StopViewDependencies interface (line 16), renderStopProperties() (line 72), addEventListeners() (line 312).
Page renderer: src/modules/page-content-renderer.ts — ContentRendererDependencies patchManager interface (line 118), stopViewDependencies construction (line 151), handleAfterRender() (line 693).
Modal utility: src/modules/modal-utils.ts — showModal({ title, body, actions }). Non-dismissible, async, buttons disabled while pending.
Navigation: src/modules/navigation-actions.ts — navigateToHome() at line 15. Calls getPageStateManager().navigateTo({ type: 'home' }) directly — no callback wiring needed.
FK relationship: Only stop_times.stop_id → stops.stop_id is relevant. stops.parent_station is a self-FK (station hierarchies) — out of scope for phase 1.
stop_times composite key: generateCompositeKeyFromRecord('stop_times', record) from src/utils/gtfs-primary-keys.ts — already used in schedule-controller.ts and timetable-database.ts.
Existing cascade pattern: src/modules/service-days-controller.ts lines 317–334 — the canonical pattern for delete + patch record: call db.deleteRow() first, then patchManager.recordDelete().
db.deleteRow: Already declared as optional in ContentRendererDependencies.gtfsDatabase (line 72 of page-content-renderer.ts). The real GTFSDatabase passed from index.ts has it — no new wiring needed.

Phase 1: Extend interfaces and add delete button to stop view

Add recordDelete to the patchManager type in PageContentRenderer, add onDeleteStop callback to StopViewDependencies, add the trash button to the stop properties header, and wire the click listener.

In src/modules/page-content-renderer.ts line ~130, add recordDelete to the patchManager interface:

recordDelete: (
  table: string,
  id: string,
  record: Record<string, unknown>
) => Promise<void>;

In src/modules/stop-view-controller.ts, add to StopViewDependencies:
```
onDeleteStop: (stop_id: string) => Promise<void>;
```

In StopViewController.renderStopProperties() (line 80), change the <h2> row to a flex row with a right-aligned trash icon button:

<div class="flex items-center justify-between">
  <h2 class="text-lg font-semibold">${renderCardLabel(...)}</h2>
  <button class="btn btn-sm btn-error btn-outline delete-stop-btn" data-stop-id="${this.currentStopId}">
    <svg ...trash icon svg...>
  </button>
</div>

Use a simple inline SVG trash icon (heroicons style, same as used in renderError()).

In StopViewController.addEventListeners() (line 312), add handler for .delete-stop-btn:

container.querySelector('.delete-stop-btn')?.addEventListener('click', async () => {
  const stop_id = btn.getAttribute('data-stop-id');
  if (stop_id) await this.dependencies.onDeleteStop(stop_id);
});

In PageContentRenderer constructor (line 151), pass onDeleteStop to stopViewDependencies:
```
onDeleteStop: (stop_id) => this.handleDeleteStop(stop_id),
```

Also updated src/modules/browse-navigation.ts to include recordDelete in its local patchManager type (needed for type compatibility). Added a stub handleDeleteStop on PageContentRenderer to be implemented in phase 2.

Gotcha: currentStopId is set in renderStopView() before renderStopProperties() is called, so it's safe to use as the data-stop-id source. Alternatively, pass stop.stop_id directly — prefer that to avoid relying on instance state.

Phase 2: Implement `handleDeleteStop` in `PageContentRenderer`

The core deletion flow: check for FK rows in stop_times, show modal if any exist, cascade-delete all or just the stop, navigate home.

Add import for showModal from ../modules/modal-utils.js in page-content-renderer.ts (if not already imported).
Add import for navigateToHome from ../modules/navigation-actions.js.
Add import for generateCompositeKeyFromRecord from ../utils/gtfs-primary-keys.js.

Add private async handleDeleteStop(stop_id: string): Promise<void> to PageContentRenderer:

private async handleDeleteStop(stop_id: string): Promise<void> {
  const db = this.dependencies.gtfsDatabase;
  const pm = this.dependencies.patchManager;
  if (!db || !pm || !db.deleteRow) return;

  // Fetch the stop record (needed for recordDelete's inverse payload)
  const stops = await db.queryRows('stops', { stop_id });
  const stop = stops[0] as Record<string, unknown> | undefined;
  if (!stop) return;

  // Check for FK references in stop_times
  const stopTimes = await db.queryRows('stop_times', { stop_id }) as Record<string, unknown>[];

  const doDelete = async (cascade: boolean) => {
    if (cascade) {
      for (const st of stopTimes) {
        const key = generateCompositeKeyFromRecord('stop_times', st);
        await db.deleteRow!('stop_times', key);
        await pm.recordDelete('stop_times', key, st);
      }
    }
    await db.deleteRow!('stops', stop_id);
    await pm.recordDelete('stops', stop_id, stop);
    console.log(`[PageContentRenderer] Deleted stop ${stop_id}${cascade ? ` and ${stopTimes.length} stop_times` : ''}`);
    await navigateToHome();
  };

  if (stopTimes.length === 0) {
    await doDelete(false);
    return;
  }

  // FK conflict — show modal
  const tripIds = [...new Set(stopTimes.map(st => st.trip_id as string))];
  const tripSummary = tripIds.slice(0, 5).join(', ') + (tripIds.length > 5 ? ` … and ${tripIds.length - 5} more` : '');
  await showModal({
    title: 'Stop has scheduled visits',
    body: `<p>This stop is referenced by <strong>${stopTimes.length} stop_time${stopTimes.length !== 1 ? 's' : ''}</strong> across ${tripIds.length} trip${tripIds.length !== 1 ? 's' : ''}:</p>
           <p class="text-sm opacity-70 mt-1">${tripSummary}</p>
           <p class="mt-3">You can cascade-delete the stop and all its stop_times (reversible via undo), or cancel.</p>`,
    actions: [
      { label: 'Cancel', className: 'btn-ghost', onClick: () => {} },
      {
        label: `Delete stop + ${stopTimes.length} stop_times`,
        className: 'btn-error',
        onClick: () => doDelete(true),
      },
    ],
  });
}

Gotcha: doDelete is called inside the modal onClick which showModal awaits — navigateToHome() runs after showModal resolves and the modal is removed from DOM. This is fine because showModal resolves after onClick completes.

Gotcha: generateCompositeKeyFromRecord expects the raw DB record object, which is what queryRows returns (shallow copies per the virtual-table invariant). This is safe.

Gotcha: Each stop_time gets its own recordDelete patch entry so each is individually revertable from the Changes panel, and a full undo of the stop's delete re-inserts just the stop (not the stop_times). To undo the cascade, the user would revert each stop_time patch individually. This is intentional — batch undo is a separate future feature.

Phase 3: Fix silent delete button (nothing happens on click)

The button renders but clicking it produces no console output and no action. The chain has three silent failure points that need logs added and one structural fix.

Root causes to address:

No logging in the click handler — can't tell if the listener is attached or if stop_id is null/empty
No logging in handleDeleteStop — the early return on !db || !pm || !db.deleteRow is completely silent
The querySelector approach is fragile — if the button isn't in the container at listener-attach time (e.g. the container ref is stale), no listener gets attached and nothing fires

Fix: switch to event delegation + add logs throughout the chain.

In src/modules/stop-view-controller.ts addEventListeners(), replace the querySelector approach with event delegation on the container:

container.addEventListener('click', async (e) => {
  const btn = (e.target as Element).closest('.delete-stop-btn');
  if (!btn) return;
  console.log('[StopViewController] Delete button clicked');
  const stop_id = btn.getAttribute('data-stop-id');
  console.log('[StopViewController] stop_id from button:', stop_id);
  if (stop_id) await this.dependencies.onDeleteStop(stop_id);
});

This is robust to clicks on the SVG child element and doesn't depend on the button being in the DOM at attach time.

In src/modules/page-content-renderer.ts handleDeleteStop(), add a log at the very top and after each early-return guard:

console.log('[PageContentRenderer] handleDeleteStop called, stop_id:', stop_id);
const db = this.dependencies.gtfsDatabase;
const pm = this.dependencies.patchManager;
if (!db || !pm || !db.deleteRow) {
  console.warn('[PageContentRenderer] handleDeleteStop: missing db/pm/deleteRow', { db: !!db, pm: !!pm, deleteRow: !!db?.deleteRow });
  return;
}

And after the stop lookup:

if (!stop) {
  console.warn('[PageContentRenderer] handleDeleteStop: stop not found for id', stop_id);
  return;
}

Gotcha: Event delegation on the container means the listener fires for all clicks inside the browse panel — closest('.delete-stop-btn') guards correctly so non-delete clicks are ignored immediately.

Gotcha: The patchManager in ContentRendererDependencies may be passed as undefined if the feed isn't loaded yet. The new warn log will surface this immediately.

Phase 4: Wire `deleteRow` proxy and refresh map after deletion

Two bugs surfaced in testing: (1) the gtfsDatabase proxy object in browse-navigation.ts was missing deleteRow, so the guard in handleDeleteStop always bailed early with deleteRow: false; (2) after a successful deletion the stop marker remained visible on the map because nothing called layerManager.updateStopsData().

In src/modules/browse-navigation.ts initializeContentRenderer(), add deleteRow to the gtfsDatabase proxy object, delegating to this.gtfsRelationshipsInstance.gtfsDatabase.deleteRow.
Add public refreshStops(): void to MapController in src/modules/map-controller.ts that calls this.layerManager?.updateStopsData().
Add refreshStops: () => void to the mapController interface in ContentRendererDependencies in src/modules/page-content-renderer.ts.
Add refreshStops to both the private field type and constructor parameter type for mapController in src/modules/browse-navigation.ts.
Wire refreshStops: () => this.mapController.refreshStops() in the mapController proxy in initializeContentRenderer().
Call this.dependencies.mapController.refreshStops() in doDelete() inside handleDeleteStop, just before navigateToHome().

Discovery: recordDelete in patch-manager.ts does NOT call applyPatchForward (unlike recordUpdate). The actual DB row removal happens via the direct db.deleteRow call before recordDelete is invoked, so the virtual-table in-memory data is already updated when refreshStops / updateStopsData runs — gtfsParser.getFileDataSyncTyped('stops.txt') returns the correct post-deletion list.

Phase 5: Fix multiple modals on delete (accumulated event listeners)

Root cause: addEventListeners is called on every render of the stop view. The delete handler uses event delegation on the container element, but the container persists across renders — only its innerHTML is replaced. Each call to addEventListeners stacks another click listener on the same container node, so clicking delete fires once per accumulated listener, spawning one modal per prior render.

Add private deleteListenerAbortController: AbortController | null = null to StopViewController.
In addEventListeners, before attaching the delete click listener, abort the previous controller (this.deleteListenerAbortController?.abort()) and create a new one.
Pass { signal: this.deleteListenerAbortController.signal } as the third argument to container.addEventListener, so the previous listener is automatically removed when the controller is aborted on the next render.

Why AbortController over a boolean flag: A flag would prevent re-attaching when the container changes (e.g. navigating to a different stop in the same panel); AbortController correctly removes the old listener from whatever container it was on and registers a fresh one on the current container.

Original Issue

Stops should be a good place to start.

## Summary Add the ability to delete GTFS objects, starting with stops. The patch system already fully supports delete operations — `patchManager.recordDelete()`, undo/redo replay, and the history UI red badge are all in place. The work is purely in wiring a delete button to the stop view UI, implementing a FK-aware confirmation modal (showing affected `stop_times` with an option to cascade-delete them all), and navigating home after deletion. The cascade pattern is borrowed from the existing `removeException` flow in `service-days-controller.ts`. **Key tradeoff**: Instead of adding mutation logic to `StopViewController` (which is a view-only component), we use a callback pattern (`onDeleteStop`) so all DB writes stay in `PageContentRenderer`. This preserves the architectural boundary. ## Relevant Context - **Patch system**: `src/modules/patch-manager.ts` — `recordDelete(table, id, record)` at line 194. Already full undo/redo support (inverse re-inserts from `patch.inverse.record`). - **History UI**: `src/modules/history-controller.ts` — already renders delete patches with red `badge-error`. No changes needed. - **Stop view**: `src/modules/stop-view-controller.ts` — `StopViewDependencies` interface (line 16), `renderStopProperties()` (line 72), `addEventListeners()` (line 312). - **Page renderer**: `src/modules/page-content-renderer.ts` — `ContentRendererDependencies` patchManager interface (line 118), `stopViewDependencies` construction (line 151), `handleAfterRender()` (line 693). - **Modal utility**: `src/modules/modal-utils.ts` — `showModal({ title, body, actions })`. Non-dismissible, async, buttons disabled while pending. - **Navigation**: `src/modules/navigation-actions.ts` — `navigateToHome()` at line 15. Calls `getPageStateManager().navigateTo({ type: 'home' })` directly — no callback wiring needed. - **FK relationship**: Only `stop_times.stop_id → stops.stop_id` is relevant. `stops.parent_station` is a self-FK (station hierarchies) — out of scope for phase 1. - **stop_times composite key**: `generateCompositeKeyFromRecord('stop_times', record)` from `src/utils/gtfs-primary-keys.ts` — already used in `schedule-controller.ts` and `timetable-database.ts`. - **Existing cascade pattern**: `src/modules/service-days-controller.ts` lines 317–334 — the canonical pattern for delete + patch record: call `db.deleteRow()` first, then `patchManager.recordDelete()`. - **`db.deleteRow`**: Already declared as optional in `ContentRendererDependencies.gtfsDatabase` (line 72 of `page-content-renderer.ts`). The real `GTFSDatabase` passed from `index.ts` has it — no new wiring needed. --- ## Phase 1: Extend interfaces and add delete button to stop view Add `recordDelete` to the `patchManager` type in `PageContentRenderer`, add `onDeleteStop` callback to `StopViewDependencies`, add the trash button to the stop properties header, and wire the click listener. - [x] In `src/modules/page-content-renderer.ts` line ~130, add `recordDelete` to the `patchManager` interface: ```ts recordDelete: ( table: string, id: string, record: Record<string, unknown> ) => Promise<void>; ``` - [x] In `src/modules/stop-view-controller.ts`, add to `StopViewDependencies`: ```ts onDeleteStop: (stop_id: string) => Promise<void>; ``` - [x] In `StopViewController.renderStopProperties()` (line 80), change the `<h2>` row to a flex row with a right-aligned trash icon button: ```html <div class="flex items-center justify-between"> <h2 class="text-lg font-semibold">${renderCardLabel(...)}</h2> <button class="btn btn-sm btn-error btn-outline delete-stop-btn" data-stop-id="${this.currentStopId}"> <svg ...trash icon svg...> </button> </div> ``` Use a simple inline SVG trash icon (heroicons style, same as used in `renderError()`). - [x] In `StopViewController.addEventListeners()` (line 312), add handler for `.delete-stop-btn`: ```ts container.querySelector('.delete-stop-btn')?.addEventListener('click', async () => { const stop_id = btn.getAttribute('data-stop-id'); if (stop_id) await this.dependencies.onDeleteStop(stop_id); }); ``` - [x] In `PageContentRenderer` constructor (line 151), pass `onDeleteStop` to `stopViewDependencies`: ```ts onDeleteStop: (stop_id) => this.handleDeleteStop(stop_id), ``` Also updated `src/modules/browse-navigation.ts` to include `recordDelete` in its local `patchManager` type (needed for type compatibility). Added a stub `handleDeleteStop` on `PageContentRenderer` to be implemented in phase 2. **Gotcha**: `currentStopId` is set in `renderStopView()` before `renderStopProperties()` is called, so it's safe to use as the `data-stop-id` source. Alternatively, pass `stop.stop_id` directly — prefer that to avoid relying on instance state. --- ## Phase 2: Implement `handleDeleteStop` in `PageContentRenderer` The core deletion flow: check for FK rows in `stop_times`, show modal if any exist, cascade-delete all or just the stop, navigate home. - [x] Add import for `showModal` from `../modules/modal-utils.js` in `page-content-renderer.ts` (if not already imported). - [x] Add import for `navigateToHome` from `../modules/navigation-actions.js`. - [x] Add import for `generateCompositeKeyFromRecord` from `../utils/gtfs-primary-keys.js`. - [x] Add `private async handleDeleteStop(stop_id: string): Promise<void>` to `PageContentRenderer`: ```ts private async handleDeleteStop(stop_id: string): Promise<void> { const db = this.dependencies.gtfsDatabase; const pm = this.dependencies.patchManager; if (!db || !pm || !db.deleteRow) return; // Fetch the stop record (needed for recordDelete's inverse payload) const stops = await db.queryRows('stops', { stop_id }); const stop = stops[0] as Record<string, unknown> | undefined; if (!stop) return; // Check for FK references in stop_times const stopTimes = await db.queryRows('stop_times', { stop_id }) as Record<string, unknown>[]; const doDelete = async (cascade: boolean) => { if (cascade) { for (const st of stopTimes) { const key = generateCompositeKeyFromRecord('stop_times', st); await db.deleteRow!('stop_times', key); await pm.recordDelete('stop_times', key, st); } } await db.deleteRow!('stops', stop_id); await pm.recordDelete('stops', stop_id, stop); console.log(`[PageContentRenderer] Deleted stop ${stop_id}${cascade ? ` and ${stopTimes.length} stop_times` : ''}`); await navigateToHome(); }; if (stopTimes.length === 0) { await doDelete(false); return; } // FK conflict — show modal const tripIds = [...new Set(stopTimes.map(st => st.trip_id as string))]; const tripSummary = tripIds.slice(0, 5).join(', ') + (tripIds.length > 5 ? ` … and ${tripIds.length - 5} more` : ''); await showModal({ title: 'Stop has scheduled visits', body: `<p>This stop is referenced by <strong>${stopTimes.length} stop_time${stopTimes.length !== 1 ? 's' : ''}</strong> across ${tripIds.length} trip${tripIds.length !== 1 ? 's' : ''}:</p> <p class="text-sm opacity-70 mt-1">${tripSummary}</p> <p class="mt-3">You can cascade-delete the stop and all its stop_times (reversible via undo), or cancel.</p>`, actions: [ { label: 'Cancel', className: 'btn-ghost', onClick: () => {} }, { label: `Delete stop + ${stopTimes.length} stop_times`, className: 'btn-error', onClick: () => doDelete(true), }, ], }); } ``` **Gotcha**: `doDelete` is called inside the modal `onClick` which `showModal` awaits — `navigateToHome()` runs after `showModal` resolves and the modal is removed from DOM. This is fine because `showModal` resolves after `onClick` completes. **Gotcha**: `generateCompositeKeyFromRecord` expects the raw DB record object, which is what `queryRows` returns (shallow copies per the virtual-table invariant). This is safe. **Gotcha**: Each `stop_time` gets its own `recordDelete` patch entry so each is individually revertable from the Changes panel, and a full undo of the stop's delete re-inserts just the stop (not the stop_times). To undo the cascade, the user would revert each stop_time patch individually. This is intentional — batch undo is a separate future feature. --- ## Phase 3: Fix silent delete button (nothing happens on click) The button renders but clicking it produces no console output and no action. The chain has three silent failure points that need logs added and one structural fix. **Root causes to address:** 1. No logging in the click handler — can't tell if the listener is attached or if `stop_id` is null/empty 2. No logging in `handleDeleteStop` — the early `return` on `!db || !pm || !db.deleteRow` is completely silent 3. The querySelector approach is fragile — if the button isn't in the container at listener-attach time (e.g. the container ref is stale), no listener gets attached and nothing fires **Fix: switch to event delegation + add logs throughout the chain.** - [x] In `src/modules/stop-view-controller.ts` `addEventListeners()`, replace the `querySelector` approach with event delegation on the container: ```ts container.addEventListener('click', async (e) => { const btn = (e.target as Element).closest('.delete-stop-btn'); if (!btn) return; console.log('[StopViewController] Delete button clicked'); const stop_id = btn.getAttribute('data-stop-id'); console.log('[StopViewController] stop_id from button:', stop_id); if (stop_id) await this.dependencies.onDeleteStop(stop_id); }); ``` This is robust to clicks on the SVG child element and doesn't depend on the button being in the DOM at attach time. - [x] In `src/modules/page-content-renderer.ts` `handleDeleteStop()`, add a log at the very top and after each early-return guard: ```ts console.log('[PageContentRenderer] handleDeleteStop called, stop_id:', stop_id); const db = this.dependencies.gtfsDatabase; const pm = this.dependencies.patchManager; if (!db || !pm || !db.deleteRow) { console.warn('[PageContentRenderer] handleDeleteStop: missing db/pm/deleteRow', { db: !!db, pm: !!pm, deleteRow: !!db?.deleteRow }); return; } ``` And after the stop lookup: ```ts if (!stop) { console.warn('[PageContentRenderer] handleDeleteStop: stop not found for id', stop_id); return; } ``` **Gotcha**: Event delegation on the container means the listener fires for all clicks inside the browse panel — `closest('.delete-stop-btn')` guards correctly so non-delete clicks are ignored immediately. **Gotcha**: The `patchManager` in `ContentRendererDependencies` may be passed as `undefined` if the feed isn't loaded yet. The new warn log will surface this immediately. --- ## Phase 4: Wire `deleteRow` proxy and refresh map after deletion Two bugs surfaced in testing: (1) the `gtfsDatabase` proxy object in `browse-navigation.ts` was missing `deleteRow`, so the guard in `handleDeleteStop` always bailed early with `deleteRow: false`; (2) after a successful deletion the stop marker remained visible on the map because nothing called `layerManager.updateStopsData()`. - [x] In `src/modules/browse-navigation.ts` `initializeContentRenderer()`, add `deleteRow` to the `gtfsDatabase` proxy object, delegating to `this.gtfsRelationshipsInstance.gtfsDatabase.deleteRow`. - [x] Add `public refreshStops(): void` to `MapController` in `src/modules/map-controller.ts` that calls `this.layerManager?.updateStopsData()`. - [x] Add `refreshStops: () => void` to the `mapController` interface in `ContentRendererDependencies` in `src/modules/page-content-renderer.ts`. - [x] Add `refreshStops` to both the private field type and constructor parameter type for `mapController` in `src/modules/browse-navigation.ts`. - [x] Wire `refreshStops: () => this.mapController.refreshStops()` in the `mapController` proxy in `initializeContentRenderer()`. - [x] Call `this.dependencies.mapController.refreshStops()` in `doDelete()` inside `handleDeleteStop`, just before `navigateToHome()`. **Discovery**: `recordDelete` in `patch-manager.ts` does NOT call `applyPatchForward` (unlike `recordUpdate`). The actual DB row removal happens via the direct `db.deleteRow` call before `recordDelete` is invoked, so the virtual-table in-memory data is already updated when `refreshStops` / `updateStopsData` runs — `gtfsParser.getFileDataSyncTyped('stops.txt')` returns the correct post-deletion list. --- ## Phase 5: Fix multiple modals on delete (accumulated event listeners) **Root cause**: `addEventListeners` is called on every render of the stop view. The delete handler uses event delegation on the `container` element, but the container persists across renders — only its `innerHTML` is replaced. Each call to `addEventListeners` stacks another `click` listener on the same container node, so clicking delete fires once per accumulated listener, spawning one modal per prior render. - [x] Add `private deleteListenerAbortController: AbortController | null = null` to `StopViewController`. - [x] In `addEventListeners`, before attaching the delete click listener, abort the previous controller (`this.deleteListenerAbortController?.abort()`) and create a new one. - [x] Pass `{ signal: this.deleteListenerAbortController.signal }` as the third argument to `container.addEventListener`, so the previous listener is automatically removed when the controller is aborted on the next render. **Why AbortController over a boolean flag**: A flag would prevent re-attaching when the container changes (e.g. navigating to a different stop in the same panel); `AbortController` correctly removes the old listener from whatever container it was on and registers a fresh one on the current container. --- ## Original Issue Stops should be a good place to start.

maxtkc added the

Kind/Feature

Priority

Medium

labels

2026-03-29 22:26:58 +00:00

maxtkc self-assigned this

2026-03-29 22:26:58 +00:00

maxtkc added this to the edit.gtfs.zone project

2026-03-29 22:26:58 +00:00

maxtkc commented

2026-04-09 00:04:39 +00:00

Author

Owner

Plan: Fix cascade delete — batch patch, sticking notifications, and console warnings

Summary

Follow-on to #48. Three bugs remain after the delete-stop feature landed. All three share the same root cause: the doDelete(cascade: true) path in handleDeleteStop calls pm.recordDelete() separately for each stop_time and the stop, producing N+1 appendAndPush calls → N+1 'change' events → N+1 notifications and N+1 async browseNavigation.refresh() calls. The fix is to batch all DB deletions, then record one BatchGTFSPatch with a single appendAndPush. As a secondary fix, revertPatch in patch-manager.ts incorrectly hardcodes op: 'update' when inverting batch patches (harmless today since batches only contain updates, but would silently break for our new delete batches). Cosmetic: renderFieldDiffs in history-controller.ts says "N rows updated" for all batches — should reflect the actual op type.

Relevant context

PatchManager.recordBatch (src/modules/patch-manager.ts:208) — the existing batch method for update ops; our new recordBatchDelete follows the same pattern but for delete ops. appendAndPush is private so the new method must live on PatchManager.
BatchGTFSPatch (src/types/patch.ts:22) — ops: SingleGTFSPatch[]. The type already accepts any op including 'delete' — no type change needed.
applyPatchInverse for batch (patch-manager.ts:124) — correctly handles any op type by dispatching on patch.op; undo works fine. The bug is only in revertPatch.
revertPatch for batch (patch-manager.ts:307-319) — hardcodes op: 'update' as const for all ops when building the inverted batch. Works for update batches (the only kind that exist today), but would silently fail for delete batches (inverted payload shape { record: ... } doesn't match the update branch which expects { changes: ... }).
handleDeleteStop (src/modules/page-content-renderer.ts:813) — the cascade path runs a for-loop calling db.deleteRow + pm.recordDelete per stop_time, then the same for the stop itself.
patchManager type in page-content-renderer.ts (line 122) and in browse-navigation.ts (line 95 + setPatchManager at line 114) — both need recordBatchDelete added.
humanLabel (src/utils/patch-label.ts:7) — uses patch.label for batch patches; no change needed if we pass a descriptive label.
renderFieldDiffs for batch (src/modules/history-controller.ts:44) — hardcoded "rows updated" string.

Why the batch approach fixes all three bugs

N+1 patches → 1 patch: One appendAndPush call instead of N+1.
Sticking notifications: One 'change' event → one notifications.showInfo call → one notification that auto-dismisses.
Console warnings: One async browseNavigation.refresh() queued. By the time the microtask queue runs it, navigateToHome() has already changed the page state to 'home', so refresh() calls render() which renders the home view — NOT renderStop(stop_id), so highlightStop is never called on the deleted stop. The getStopTimesByStopId: index miss also disappears because getAgenciesServingStop is never invoked for the deleted stop.

Phase 1: Add `recordBatchDelete` to PatchManager + fix `revertPatch`

Add the new batch-delete recording method and fix the revert-patch inversion bug while we're in the file.

In src/modules/patch-manager.ts, add after recordDelete (line 206):

async recordBatchDelete(
  ops: Array<{ table: string; id: string; record: Record<string, unknown> }>,
  label?: string
): Promise<void> {
  if (ops.length === 0) return;
  const singlePatches: SingleGTFSPatch[] = ops.map(({ table, id, record }) => ({
    op: 'delete' as const,
    source: { table, id },
    forward: { id },
    inverse: { record },
  }));
  if (singlePatches.length === 1) {
    await this.appendAndPush(singlePatches[0]);
    return;
  }
  const batchPatch: BatchGTFSPatch = {
    op: 'batch',
    ops: singlePatches,
    label,
  };
  await this.appendAndPush(batchPatch);
}

Note: do NOT call applyPatchForward here — the DB deletions will have already happened via direct db.deleteRow calls before this method is invoked (same contract as the existing recordDelete).

Fix revertPatch batch inversion (patch-manager.ts around line 308). Replace the hardcoded op: 'update' as const with correct inversion:

ops: patch.ops.map((op) => {
  if (op.op === 'delete') {
    return { op: 'insert' as const, source: op.source, forward: op.inverse, inverse: op.forward };
  } else if (op.op === 'insert') {
    return { op: 'delete' as const, source: op.source, forward: op.inverse, inverse: op.forward };
  } else {
    return { op: 'update' as const, source: op.source, forward: op.inverse, inverse: op.forward };
  }
}),

Gotcha: recordBatchDelete intentionally skips applyPatchForward. The virtual table's delete handler would handle a double-delete safely (early-return if row not in byId), but skipping is cleaner and consistent with recordDelete.

Phase 2: Update `handleDeleteStop` + add `recordBatchDelete` to interface types

Wire recordBatchDelete into the cascade delete flow and extend the two type-narrowed patchManager shapes.

In src/modules/page-content-renderer.ts patchManager type (lines 122–139), add:

recordBatchDelete: (
  ops: Array<{ table: string; id: string; record: Record<string, unknown> }>,
  label?: string
) => Promise<void>;

In src/modules/browse-navigation.ts, add the same recordBatchDelete signature to:
- The private field type at line 95
- The setPatchManager parameter type at line 114

In src/modules/page-content-renderer.ts handleDeleteStop, replace the doDelete inner function's cascade path:

Current:

const doDelete = async (cascade: boolean) => {
  if (cascade) {
    for (const st of stopTimes) {
      const key = generateCompositeKeyFromRecord('stop_times', st);
      await db.deleteRow!('stop_times', key);
      await pm.recordDelete('stop_times', key, st);
    }
  }
  await db.deleteRow!('stops', stop_id);
  await pm.recordDelete('stops', stop_id, stop);
  ...
};

Replacement:

const doDelete = async (cascade: boolean) => {
  if (cascade) {
    // Do all DB deletions first — no 'change' events yet
    for (const st of stopTimes) {
      const key = generateCompositeKeyFromRecord('stop_times', st);
      await db.deleteRow!('stop_times', key);
    }
  }
  await db.deleteRow!('stops', stop_id);

  // Record as one atomic batch patch → one 'change' event, one notification
  const deleteOps = [
    ...(cascade
      ? stopTimes.map((st) => ({
          table: 'stop_times',
          id: generateCompositeKeyFromRecord('stop_times', st),
          record: st,
        }))
      : []),
    { table: 'stops', id: stop_id, record: stop },
  ];
  const label = cascade
    ? `Delete stop + ${stopTimes.length} stop_time${stopTimes.length !== 1 ? 's' : ''}`
    : 'Delete stop';
  await pm.recordBatchDelete(deleteOps, label);

  console.log(
    `[PageContentRenderer] Deleted stop ${stop_id}${cascade ? ` and ${stopTimes.length} stop_times` : ''}`
  );
  this.dependencies.mapController.refreshStops();
  await navigateToHome();
};

Gotcha: pm.recordBatchDelete with a single-element deleteOps (the no-cascade path) will fall through to the singlePatches.length === 1 branch in recordBatchDelete and record a plain SingleGTFSPatch — identical behavior to the old pm.recordDelete call. This is fine. You could keep the non-cascade path using pm.recordDelete directly, but using recordBatchDelete uniformly simplifies the logic.

Gotcha: The doDelete closure captures stopTimes by reference. The stop_times are fetched before the modal is shown (if any), so stopTimes is stable. No re-query needed.

Phase 3: Fix history UI batch label for non-update ops (cosmetic)

renderFieldDiffs in history-controller.ts says "N rows updated" for all batch patches. Update it to reflect the actual operation type.

In src/modules/history-controller.ts, update the 'batch' branch of renderFieldDiffs (line 44):

if (patch.op === 'batch') {
  const ops = patch.ops;
  const opTypes = new Set(ops.map((op) => op.op));
  let verb: string;
  if (opTypes.size === 1) {
    const t = opTypes.values().next().value;
    verb = t === 'insert' ? 'inserted' : t === 'delete' ? 'deleted' : 'updated';
  } else {
    verb = 'changed';
  }
  return `<div class="text-xs mt-0.5">${ops.length} rows ${verb}</div>`;
}

Gotcha: This is purely cosmetic — the patch data is correct regardless. The opBadgeClass function already uses 'badge-info' for all batches (no change there — mixed-op batches are fine as info color).

# Plan: Fix cascade delete — batch patch, sticking notifications, and console warnings ## Summary Follow-on to #48. Three bugs remain after the delete-stop feature landed. All three share the same root cause: the `doDelete(cascade: true)` path in `handleDeleteStop` calls `pm.recordDelete()` separately for each stop_time and the stop, producing N+1 `appendAndPush` calls → N+1 `'change'` events → N+1 notifications and N+1 async `browseNavigation.refresh()` calls. The fix is to batch all DB deletions, then record one `BatchGTFSPatch` with a single `appendAndPush`. As a secondary fix, `revertPatch` in `patch-manager.ts` incorrectly hardcodes `op: 'update'` when inverting batch patches (harmless today since batches only contain updates, but would silently break for our new delete batches). Cosmetic: `renderFieldDiffs` in `history-controller.ts` says "N rows updated" for all batches — should reflect the actual op type. ## Relevant context - **`PatchManager.recordBatch`** (`src/modules/patch-manager.ts:208`) — the existing batch method for update ops; our new `recordBatchDelete` follows the same pattern but for delete ops. `appendAndPush` is private so the new method must live on `PatchManager`. - **`BatchGTFSPatch`** (`src/types/patch.ts:22`) — `ops: SingleGTFSPatch[]`. The type already accepts any op including `'delete'` — no type change needed. - **`applyPatchInverse` for batch** (`patch-manager.ts:124`) — correctly handles any op type by dispatching on `patch.op`; undo works fine. The bug is only in `revertPatch`. - **`revertPatch` for batch** (`patch-manager.ts:307-319`) — hardcodes `op: 'update' as const` for all ops when building the inverted batch. Works for update batches (the only kind that exist today), but would silently fail for delete batches (inverted payload shape `{ record: ... }` doesn't match the `update` branch which expects `{ changes: ... }`). - **`handleDeleteStop`** (`src/modules/page-content-renderer.ts:813`) — the cascade path runs a for-loop calling `db.deleteRow` + `pm.recordDelete` per stop_time, then the same for the stop itself. - **`patchManager` type in `page-content-renderer.ts`** (line 122) and **in `browse-navigation.ts`** (line 95 + `setPatchManager` at line 114) — both need `recordBatchDelete` added. - **`humanLabel`** (`src/utils/patch-label.ts:7`) — uses `patch.label` for batch patches; no change needed if we pass a descriptive label. - **`renderFieldDiffs` for batch** (`src/modules/history-controller.ts:44`) — hardcoded `"rows updated"` string. ## Why the batch approach fixes all three bugs 1. **N+1 patches → 1 patch**: One `appendAndPush` call instead of N+1. 2. **Sticking notifications**: One `'change'` event → one `notifications.showInfo` call → one notification that auto-dismisses. 3. **Console warnings**: One async `browseNavigation.refresh()` queued. By the time the microtask queue runs it, `navigateToHome()` has already changed the page state to `'home'`, so `refresh()` calls `render()` which renders the home view — NOT `renderStop(stop_id)`, so `highlightStop` is never called on the deleted stop. The `getStopTimesByStopId: index miss` also disappears because `getAgenciesServingStop` is never invoked for the deleted stop. --- ## Phase 1: Add `recordBatchDelete` to PatchManager + fix `revertPatch` Add the new batch-delete recording method and fix the revert-patch inversion bug while we're in the file. - [ ] In `src/modules/patch-manager.ts`, add after `recordDelete` (line 206): ```ts async recordBatchDelete( ops: Array<{ table: string; id: string; record: Record<string, unknown> }>, label?: string ): Promise<void> { if (ops.length === 0) return; const singlePatches: SingleGTFSPatch[] = ops.map(({ table, id, record }) => ({ op: 'delete' as const, source: { table, id }, forward: { id }, inverse: { record }, })); if (singlePatches.length === 1) { await this.appendAndPush(singlePatches[0]); return; } const batchPatch: BatchGTFSPatch = { op: 'batch', ops: singlePatches, label, }; await this.appendAndPush(batchPatch); } ``` Note: do NOT call `applyPatchForward` here — the DB deletions will have already happened via direct `db.deleteRow` calls before this method is invoked (same contract as the existing `recordDelete`). - [ ] Fix `revertPatch` batch inversion (`patch-manager.ts` around line 308). Replace the hardcoded `op: 'update' as const` with correct inversion: ```ts ops: patch.ops.map((op) => { if (op.op === 'delete') { return { op: 'insert' as const, source: op.source, forward: op.inverse, inverse: op.forward }; } else if (op.op === 'insert') { return { op: 'delete' as const, source: op.source, forward: op.inverse, inverse: op.forward }; } else { return { op: 'update' as const, source: op.source, forward: op.inverse, inverse: op.forward }; } }), ``` **Gotcha**: `recordBatchDelete` intentionally skips `applyPatchForward`. The virtual table's `delete` handler would handle a double-delete safely (early-return if row not in `byId`), but skipping is cleaner and consistent with `recordDelete`. --- ## Phase 2: Update `handleDeleteStop` + add `recordBatchDelete` to interface types Wire `recordBatchDelete` into the cascade delete flow and extend the two type-narrowed `patchManager` shapes. - [ ] In `src/modules/page-content-renderer.ts` `patchManager` type (lines 122–139), add: ```ts recordBatchDelete: ( ops: Array<{ table: string; id: string; record: Record<string, unknown> }>, label?: string ) => Promise<void>; ``` - [ ] In `src/modules/browse-navigation.ts`, add the same `recordBatchDelete` signature to: - The private field type at line 95 - The `setPatchManager` parameter type at line 114 - [ ] In `src/modules/page-content-renderer.ts` `handleDeleteStop`, replace the `doDelete` inner function's cascade path: **Current:** ```ts const doDelete = async (cascade: boolean) => { if (cascade) { for (const st of stopTimes) { const key = generateCompositeKeyFromRecord('stop_times', st); await db.deleteRow!('stop_times', key); await pm.recordDelete('stop_times', key, st); } } await db.deleteRow!('stops', stop_id); await pm.recordDelete('stops', stop_id, stop); ... }; ``` **Replacement:** ```ts const doDelete = async (cascade: boolean) => { if (cascade) { // Do all DB deletions first — no 'change' events yet for (const st of stopTimes) { const key = generateCompositeKeyFromRecord('stop_times', st); await db.deleteRow!('stop_times', key); } } await db.deleteRow!('stops', stop_id); // Record as one atomic batch patch → one 'change' event, one notification const deleteOps = [ ...(cascade ? stopTimes.map((st) => ({ table: 'stop_times', id: generateCompositeKeyFromRecord('stop_times', st), record: st, })) : []), { table: 'stops', id: stop_id, record: stop }, ]; const label = cascade ? `Delete stop + ${stopTimes.length} stop_time${stopTimes.length !== 1 ? 's' : ''}` : 'Delete stop'; await pm.recordBatchDelete(deleteOps, label); console.log( `[PageContentRenderer] Deleted stop ${stop_id}${cascade ? ` and ${stopTimes.length} stop_times` : ''}` ); this.dependencies.mapController.refreshStops(); await navigateToHome(); }; ``` **Gotcha**: `pm.recordBatchDelete` with a single-element `deleteOps` (the no-cascade path) will fall through to the `singlePatches.length === 1` branch in `recordBatchDelete` and record a plain `SingleGTFSPatch` — identical behavior to the old `pm.recordDelete` call. This is fine. You could keep the non-cascade path using `pm.recordDelete` directly, but using `recordBatchDelete` uniformly simplifies the logic. **Gotcha**: The `doDelete` closure captures `stopTimes` by reference. The stop_times are fetched before the modal is shown (if any), so `stopTimes` is stable. No re-query needed. --- ## Phase 3: Fix history UI batch label for non-update ops (cosmetic) `renderFieldDiffs` in `history-controller.ts` says "N rows updated" for all batch patches. Update it to reflect the actual operation type. - [ ] In `src/modules/history-controller.ts`, update the `'batch'` branch of `renderFieldDiffs` (line 44): ```ts if (patch.op === 'batch') { const ops = patch.ops; const opTypes = new Set(ops.map((op) => op.op)); let verb: string; if (opTypes.size === 1) { const t = opTypes.values().next().value; verb = t === 'insert' ? 'inserted' : t === 'delete' ? 'deleted' : 'updated'; } else { verb = 'changed'; } return `<div class="text-xs mt-0.5">${ops.length} rows ${verb}</div>`; } ``` **Gotcha**: This is purely cosmetic — the patch data is correct regardless. The `opBadgeClass` function already uses `'badge-info'` for all batches (no change there — mixed-op batches are fine as info color).

maxtkc referenced this issue from a commit

2026-04-09 00:05:03 +00:00

feat: add delete stop button to stop view UI (#48 phase 1)

maxtkc referenced this issue from a commit

2026-04-09 00:05:03 +00:00

feat: implement handleDeleteStop with FK-aware cascade modal (#48 phase 2)

maxtkc referenced this issue from a commit

2026-04-09 00:05:03 +00:00

fix: use event delegation for delete stop button and add debug logs (#48 phase 3)

maxtkc referenced this issue from a commit

2026-04-09 00:05:03 +00:00

feat: wire deleteRow proxy and refresh map after stop deletion (#48 phase 4)