Duplicate notifications on many creates #89
Labels
No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
gtfs.zone/coloring-book#89
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Both issues are about the toast/notification system, and they share one root cause: there is no single source of truth for change notifications. Entity changes are announced from two places at once, and the wording is inconsistent and unstyled for long values.
Created agency / agency(from the patch system's label, shown on the patchchangeevent inindex.ts) andAgency "agency" created(an explicit.show()call insrc/utils/inline-entity-creator.ts). The issue author's own suggested fix: "disable notifications from anywhere but the patch spot."STOP-97cb7d7f-7f85-41e1-9fd3-308e23da2d92) overflows the toast because the message has no word-break/hyphenation CSS. The issue also asks to revamp the messages to be consistent, include the entity name (not just the id), and synchronize the wording with the history (Changes) panel so the two don't drift.Approach (chosen with the user):
change/undo/redo handlers via one label function; remove the redundant explicit.show()success calls scattered in feature modules. This is the dedup that closes #89 and makes the double-notification bug structurally impossible.humanLabel()insrc/utils/patch-label.tsbecomes the single canonical text generator, consumed by bothindex.ts(toasts) andhistory-controller.ts(Changes panel). No drift (#135).Type "name" verb—Stop "Main St" created,Service "weekday" updated,Route "Blue Line" deleted— using the entity's display name (viasrc/utils/entity-display.ts), not the raw id.notifysingleton (notify.success/error/warning/info), replacing thenotifications.showSuccess(...)surface, the per-module injectednotificationSystem, andmap-controller's privateshowNotification.Net effect: fewer call sites, one wording source, consistent UX — "less code overall" and mistakes much harder to make.
Relevant context
Notification module —
src/modules/notification-system.tsclass NotificationSystemwithshow(message, type, options)plusshowError/showWarning/ showSuccess/showInfo/showLoading,removeNotification,removeAllNotifications,updateNotification,renderNotification,escapeHtml,initialize.export const notifications = new NotificationSystem();renderNotification()builds the message as<div class="flex-1 min-w-0"><span class="text-sm">${escapeHtml(message)}</span>...</div>inside an element withmax-w-sm; the container ismax-w-xs. No word-break → #135 overflow.The two notification emitters (the #89 duplicate)
src/index.ts(~line 237–259) listens to the patchchangeevent and callsnotifications.showInfo(humanLabel(r?.patch), { duration: 3000 }); undo/redo showUndone: ${humanLabel(...)}/Redone: ${humanLabel(...)}.humanLabelcomes fromsrc/utils/patch-label.ts.page-content-renderer.tsalready relies on this model — see its comment// Record as one atomic batch patch → one 'change' event, one notification(~line 1349).src/utils/inline-entity-creator.ts: on success calls.show('Agency "${id}" created','success')(~81), Service (~124), Route (~176) — these duplicate the patch toast. It also has validation/error.show(...,'error')calls (~35, 42, 65, 86, 108, 129, 151, 181) — those are NOT patch-driven and must be kept (re-routed throughnotify.error). Constructor takesnotificationSystem: NotificationSystem(~25) andonEntityCreated: () => void(~26); wired fromsrc/modules/browse-navigation.ts:357(onEntityCreated: () => this.render()).src/modules/schedule-controller.ts:1294:notifications.showSuccess('Trip "${id}" created successfully')— duplicate of patch toast; alsonotifications.show(...)at ~618 (audit).src/modules/map-controller.ts: a privateshowNotification()(~1183) used at ~1143, plusconsole.logfor created stop/pathway (~1157, ~1173).src/modules/ui.ts:1191:notifications.showSuccess('New empty GTFS feed created.')— this is a bulk/import action, not a single patch; keep as a deliberate one-off.Label generator —
src/utils/patch-label.tshumanLabel(patch)returns the human string. Insert case (~line 20):return \Created ${source.table} / ${source.id}`;(this is theCreated agency / agencytoast). Update case producesUpdated in /(the #135 overflow example). Delete case similar. **Rewrite this to theType "name" verb` convention** and resolve the display name.Entity display —
src/utils/entity-display.tsgetStopDisplay()(child stops →Name (stop_id); stations/standalone → name),renderOptionLabel,renderCardLabel. Reuse for stop names; add/extend a genericgetEntityDisplay(table, row)for route/agency/trip/service/pathway name resolution.History / Changes panel —
src/modules/history-controller.tshumanLabel()so panel wording == toast wording (#135 "synchronize ... so we don't have drift").Invariants to preserve
getStopDisplay()— the rewritten label must use it for stops.console.*breadcrumbs; only the user-facing duplicate toast is removed.Phases
Phase 1 — Collapse to one
notifyAPI + fix overflow (#135 mechanical)Goal: Establish a single, hard-to-misuse notification entry point and fix the long-word overflow, without yet changing which events fire. Foundation for Phases 2–3.
notification-system.ts, export the singleton asnotifywith concise methodssuccess/error/warning/info/loading(thin wrappers over the existingshow*). Optionally keepnotifications+showSuccess/etc. as deprecated aliases during the transition to limit churn, then remove them at the end of the phase.renderNotification(): addbreak-words [overflow-wrap:anywhere] hyphens-autoto the message<span>(and verifymin-w-0on its flex parent so it can shrink). Re-checkmax-w-xs/max-w-smwidths read well with wrapped ids.map-controller.ts's privateshowNotification()(~1183) and its call (~1143) withnotify.*; delete the private method.notify.*:index.ts,ui.ts,schedule-controller.ts,service-days-controller.ts,interaction-handler.ts,keyboard-shortcuts.ts,database-fallback-manager.ts,timetable-database.ts,page-content-renderer.ts,inline-entity-creator.ts.pnpm typecheck+pnpm lintclean.Gotchas:
inline-entity-creator.tsreceivesnotificationSystemvia constructor — switching it to importnotifydirectly lets us drop that constructor param and simplifybrowse-navigation.tswiring (do the param removal in Phase 3 when its success calls are also removed).Phase 1 outcome / discoveries:
show*methods to concisesuccess/error/warning/info/loading(kept the genericshow,initialize,removeNotification,updateNotification,removeAllNotifications,escapeHtml). No transitionalnotifications/show*aliases were kept — every caller was migrated in one pass, so the old names are gone.notifications.showX(→notify.x(sed across the 9 listed modules, plus the{ notifications }import/dynamic-import rename and thenotify,arg inpage-content-renderer.ts.inline-entity-creator.tsneeded no edits in Phase 1. Its calls go through the injected instance via the genericthis.notificationSystem.show(msg, 'success'|'error'), andshowis unchanged.page-content-renderer.tsnow passes thenotifysingleton into its constructor. Switching it to importnotifydirectly + dropping the constructor param is deferred to Phase 3 (as the gotcha notes), where its duplicate success.show()calls are also removed.<span>; the flex parent already hadmin-w-0. Durations/animations untouched.Phase 2 — One canonical label for toast + history (#135 consistency / no drift)
Goal: Make
humanLabel()the single source of change wording, in theType "name" verbconvention, including the entity's display name; wire the history panel to it.patch-label.ts::humanLabel(patch):${TypeLabel} "${name}" created${TypeLabel} "${name}" updated(optionally append a concise changed-field summary, e.g.Stop "Main St" updated (lat, lon)— keep it short and breakable)${TypeLabel} "${name}" deletednameviaentity-display.ts:getStopDisplay()for stops; name fields for route/agency/trip; id fallback for service (calendar) and pathway. Add a smallgetEntityDisplay(table, row)dispatcher if one doesn't exist.table+id, fall back to id (and note where a lookup would be needed) — do not introduce async DB lookups into label rendering; keephumanLabelsynchronous.history-controller.tsto render entries viahumanLabel()so the Changes panel text matches the toast exactly.index.tsundo/redo still readUndone: ${humanLabel(...)}/Redone: ...correctly with the new wording.pnpm typecheck+pnpm lintclean.Gotchas:
TypeLabelmust be human ("Stop", "Service", "Route", "Agency", "Trip", "Pathway"), not the raw table name (stops,calendar). Centralize this map inpatch-label.ts.Phase 2 outcome / discoveries:
history-controller.tsalready consumedhumanLabel(patch)(line 168) andindex.tsalready wrapped it for undo/redo (Undone:/Redone:/Undo:/Redo:) — so no edits were needed in either; rewritinghumanLabelpropagated the new wording to toast, Changes panel, and the undo/redo menu labels for free. Confirms the "single source of wording" design.getEntityDisplay(table, row)dispatcher +getTripDisplay()toentity-display.ts. The dispatcher routes agency/stops/routes/calendar/trips to their dedicated helpers and has a generic fallback (<singular>_name/<singular>_id) for everything else (pathways, transfers, levels, etc.). Callers read.primaryfor the single quoted name.TYPE_LABELSmap inpatch-label.ts(Agency/Stop/Route/Trip/Service/Pathway/…), withtypeLabel()falling back to the raw table name for unmapped tables.forward.changes), not the full row — so the name resolves fromchangesonly if the name field itself was edited; otherwise it falls back tosource.id(the documented behavior). Concretely, moving a stop still shows the long id, but now asStop "STOP-…" updated (stop_lat, stop_lon), and the Phase 1 CSS wraps the id instead of overflowing. Insert/delete carry the full record so they get proper names. A future improvement would be to thread the row name into update patches at record time, but that's out of scope (no async lookups inhumanLabel).stop_lat, stop_lon) rather than stripping the table prefix — unambiguous, breakable, and avoids a premature helper.Phase 3 — Remove duplicate emitters; patch spot is the only source (closes #89)
Goal: Ensure exactly one notification per entity change by deleting the redundant explicit success toasts; keep only non-patch notifications (validation errors, bulk import, undo/redo).
.show()/showSuccess()calls that duplicate the patch toast:inline-entity-creator.ts(agency, service, route) andschedule-controller.ts(trip created, was line 1290).inline-entity-creator.tsbut route them throughnotify.error/notify.warning; drop the now-unusednotificationSystemconstructor param and update thepage-content-renderer.tsconstruction site.schedule-controller.ts:618,service-days-controller.ts, and any other entity CRUD paths for additional explicit success toasts that duplicate a recorded patch; remove them.ui.ts(new feed / import / export),database-fallback-manager.ts(reset), pending-stop andtimetable-database.tstoasts.Agency "agency" created.pnpm typecheck+pnpm lintclean; net line count reduced.Gotchas:
page-content-renderer.tsalready follows the patch-only model; use it as the reference pattern and don't regress it.Phase 3 outcome / discoveries:
InlineEntityCreatoris constructed inpage-content-renderer.ts(notbrowse-navigation.tsas the plan guessed); theonEntityCreatedcallback flows throughPageContentRenderer's dependencies (browse-navigation.ts:357only wiresonEntityCreated: () => this.render(), which is unchanged). Dropping thenotificationSystemparam meant editing thenew InlineEntityCreator(...)call inpage-content-renderer.tsand removing its now-unusednotifyimport.inline-entity-creator.ts(agency/service/route) and one inschedule-controller.ts(trip). All four record a patch immediately before, so the patchchangeevent still emits exactly one toast — the action is not left silent.schedule-controller.ts:618(trip update failure error), the two"Stop added. Enter a time…"toasts (~1196/1382 — pending stop is UI-only state, no patch recorded),timetable-database.ts×2 (Added stop to trip— these write viadatabase.replaceRowsdirectly, bypassing the patch system, so they're the only signal for that action),database-fallback-manager.ts(DB reset), andui.ts(load file / load URL / new feed / export — all bulk/non-patch).service-days-controller.tsonly has twonotify.errorcalls.Original Issues
#89 — Duplicate notifications on many creates
#135 — Handle long words in notification