fix(notifications): dispatch webhooks for the triggers the settings screen offers - #943
Open
m4bard wants to merge 1 commit into
Open
fix(notifications): dispatch webhooks for the triggers the settings screen offers#943m4bard wants to merge 1 commit into
m4bard wants to merge 1 commit into
Conversation
…creen offers Two trigger vocabularies had grown up on either side of the same field and never met. The notification settings screen writes kebab-case names onto WebhookConfiguration.Triggers, offering book-added, book-downloading, book-available and book-completed, and the stored default matches. The code that reads that field looked for Imported, Failed, System and Moved with an ordinal List<string>.Contains, so a webhook a user created in the UI matched nothing and never fired for a real event. The Test button hid it, because the diagnostics endpoint substitutes the requested trigger for the webhook's own list and the guard therefore always passes. The kebab names were not merely unused: they were the ones production emits, from the library add, download submission and scan paths. Those call sites only ever posted to the single legacy WebhookUrl, so the webhook list the settings screen manages was unreachable from either direction. NotificationTriggers now holds the vocabulary in one place, alongside the settings that store it. Every subscription check goes through NotificationTriggers.IsEnabled, which compares case-insensitively in the same way notification payloads already compare trigger names, and resolves aliases so a webhook saved against the older internal spelling keeps working. Dispatch is unified behind SendNotificationAsync(trigger, data), which considers the legacy URL and every enabled webhook row once each, so a target configured as both does not receive an event twice. Imported maps onto book-completed. A download reaching DownloadStatus.Moved is the point at which its files have been imported into the library, which is what the settings screen labels "Processing Complete", and it was the only event with that meaning. That mapping gives book-completed a real emitter instead of an inert checkbox. Failed, Moved and System keep their own names because nothing in the settings screen means download failure, library relocation or system message; they remain dispatchable and stored rows naming them still fire, but they are still not settable from the UI. The three tests that were meant to cover this could not fail. Each built its enabled-trigger list out of the same variable it then passed as the trigger, so the guard was asked whether a name matched itself and the assertion held whatever the vocabulary was. They now pin the shipped vocabulary. The new WebhookTriggerDispatchTests asserts that a webhook subscribed to each name the settings screen offers actually produces an outbound request, that a stored name's casing does not decide whether it fires, that an unsubscribed or disabled webhook stays silent, and that the names offered by the settings screen and the names production can emit remain the same set. Retiring the legacy singular WebhookUrl, authenticating outbound webhooks and templating their bodies are all left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #872.
A webhook configured in Settings > Notifications never fired for a real event, for two independent reasons. Fixing either alone leaves the feature broken.
Vocabulary. The settings screen writes
book-added,book-downloading,book-availableandbook-completedontoWebhookConfiguration.Triggers. The dispatch guard readImported,Failed,SystemandMoved. No overlap, so the guard never passed.Reach. The five call sites emitting the kebab names posted only to the legacy singular
settings.WebhookUrland never iteratedsettings.Webhooks. So even with the vocabularies reconciled, three of the four checkboxes would still have delivered nothing.The four kebab names become canonical. They are the settings default, the migration default, what
NotificationPayloadBuilder.Attachments.csalready compares against, and what the production call sites emit, so this direction moves the fewest live values. Comparison is case-insensitive and alias-aware, so a row storing the literalImportedstill fires.book-completedgets a real emitter by mapping to the existingImportedevent, which fires when a download reachesDownloadStatus.Movedand is what the UI labels "Processing Complete". No call site was invented.Movedis deliberately not mapped there too, since it is the library relocation job and collapsing the two would double-fire and mislabel.Narrower than the issue described. #872 said option 2 also retires the legacy
WebhookUrl. This does not, because that is a design call rather than part of the defect.Behaviour change. Dispatch is unified, so an install with a legacy
WebhookUrlstored will also start receivingbook-completed, where before that target got three events. A URL that is both the legacy target and a configured row receives one delivery, not two, and there is a test for that.Tests cover each UI-settable trigger producing exactly one outbound request for its event, an unsubscribed and a disabled webhook staying silent, and stored casing not deciding whether a webhook fires. Three existing
NotificationServiceTestscases built the enabled-trigger list from the same variable they passed as the trigger, so they passed whatever the vocabulary was. Those are repaired to pin the real vocabulary.Worked through with Claude Code at my direction. The claims above were checked by running them rather than by reading, and I reviewed this before posting.