Cover useNotificationSettings, including its out-of-order-save guard - #805
Merged
Merged
Conversation
useNotificationSettings (302 lines, extracted from notification-settings.tsx by #799) had no unit coverage. Its riskiest logic is the pair of guards that keep overlapping browser-notification saves from clobbering each other: webSaveRequestIdRef gates the success path, the catch, and the finally, and the optimistic toggles roll back to the *saved* refs rather than the current ones. That is exactly the kind of logic that regresses silently. Adds 23 tests covering initial load and load failure, hasChanges across all four legs, handleSave success/failure, handleTest, the optimistic enable/event toggles and their rollbacks, three overlapping-save orderings, the visibilitychange permission re-read and its unmount teardown, and handleTestWebNotification. The overlapping-save tests drive the two toggles in separate act() calls so the ref-sync effect commits in between, matching what two real clicks produce, and assert both POST bodies. Batching them into one act() instead leaves webNotifyEnabledRef stale and posts webNotifyEnabled: false — a state the UI cannot actually reach, which would have made the tests certify the wrong contract. Verified non-vacuous: 10 source mutations, all 10 caught.
Uh oh!
There was an error while loading. Please reload this page.
selfcontained added a commit
that referenced
this pull request
Jul 22, 2026
…wiring (#810) PR #805 covered the useNotificationSettings hook; the component that wires it into the browser/Slack sections had no direct unit coverage. Add a component test that renders NotificationSettings with only its api + web-notifications seams mocked, asserting the wiring rather than re-testing hook logic: - Save gates on hasChanges and reflects the transient saving state. - A browser-notification toggle drives the browser-settings POST and its saved message renders in the Browser section. - webMessage/webError render in the Browser section and message/error in the Slack section — neither status slot cross-wires into the other (the silent bug a mis-wired prop would produce). - The enable toggle, event toggles, and test button stay behind the granted permission gate, including when a saved enabled flag outlives its permission. Verified non-vacuous against 6 source mutations (two cross-wire swaps, Save-ignores-hasChanges, checkbox/test-button permission gates) — all caught. Co-authored-by: Claude Opus 4.8 (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 freeto 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.
What
useNotificationSettings(302 lines, extracted fromnotification-settings.tsxby #799) had zero unit coverage. This adds 23 tests.The hook's riskiest logic is the set of guards that keep overlapping browser-notification saves from clobbering each other:
webSaveRequestIdRefgates the success path, thecatch, and thefinallyinpersistWebNotificationSettingssavedWebEnabledRef/savedWebEventsRef) rather than the current onesThat is exactly the kind of logic that regresses silently — nothing else in the suite touches it.
Coverage added
hasChangesacross all four legs (webhook URL, Slack events with order-insensitive comparison, and both browser-notification legs observed mid-flight)handleSavesuccess (server response wins over typed value) and failure, incl. the non-Errorrejection fallbackhandleTestsuccess /ok: false/ empty-URL omissionvisibilitychangepermission re-read, including thedocument.hiddengate and unmount teardown (with a positive control)handleTestWebNotificationgated on permissionA note on the overlapping-save tests
They drive the two toggles in separate
act()calls so the ref-sync effect commits in between, matching what two real clicks produce, and they assert both POST bodies.Batching both toggles into one
act()instead leaveswebNotifyEnabledRefstale, so the second request postswebNotifyEnabled: false— a state the UI cannot actually reach. The first draft did exactly that, and the tests passed anyway because the mocked response echoed backtrue. The review pass caught it; asserting the POST bodies is what pins the real contract down.Verification
Tests verified non-vacuous by 10 source mutations, all 10 caught — including removal of each of the three
webSaveRequestIdRefguards, swapping either rollback ref for its current-value counterpart, dropping thedocument.hiddengate, and the ref-staleness path above.One mutation deliberately left unkilled: the
.sort()on thewebNotifyEvents/savedWebEventscomparison inhasChanges. Both values are always assigned from the same source, so their order cannot diverge — it's unreachable defensive code, and a test for it would be contrived.pnpm run check,pnpm run test(server 2285 / web 268 / ext 51),pnpm run finalize:web, andpnpm run test:e2e(169 pass / 12 skip) all green.🤖 Generated with Claude Code