Skip to content

fix: rollback failed notification interaction with visual warning - #3145

Merged
afonsojramos merged 2 commits into
mainfrom
fix/state-rollback-forbidden-actions
Aug 8, 2026
Merged

fix: rollback failed notification interaction with visual warning#3145
afonsojramos merged 2 commits into
mainfrom
fix/state-rollback-forbidden-actions

Conversation

@setchy

@setchy setchy commented Aug 4, 2026

Copy link
Copy Markdown
Member

Issue
Fix a bug whereby a notification interaction can fail [rate limit, forbidden] leaving gitify in a broken state.

Manual or timer notification refreshes would not recover state, only a force reload of the app.

Example
Using a GitHub Enterprise Managed User (EMU) account, that has subscribed to a GitHub Cloud (OSS) issue or pull request.

This particular GitHub EMU setup has security policies which prevent API interactions via PAT with GitHub Cloud content.

This throws a HTTP 403 Forbidden when performing mark as read, mark as done or unsubscribe interactions.

Before

before.mov

After
Notification state restored on failure [tanstack query]
Visual indicator on notification interaction buttons about reason for failure

Screen.Recording.2026-08-04.at.8.08.04.AM.mov

@setchy
setchy requested a review from afonsojramos as a code owner August 4, 2026 12:59
@github-actions github-actions Bot added the bug Something isn't working label Aug 4, 2026
Signed-off-by: Adam Setch <adam.setch@outlook.com>
@afonsojramos
afonsojramos force-pushed the fix/state-rollback-forbidden-actions branch from f6b4c89 to 0688c10 Compare August 6, 2026 20:47
@setchy

setchy commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

@codebytere - this may help with some of the occasional random state corruption you've previously reported

Comment thread src/renderer/hooks/useNotifications.ts Outdated
Comment thread src/renderer/utils/notifications/mutations.ts Outdated
Comment thread src/renderer/components/notifications/NotificationRow.tsx Outdated
Comment thread src/renderer/stores/useNotificationActionFailuresStore.ts Outdated
Comment thread src/renderer/hooks/useNotifications.ts Outdated
Comment thread src/renderer/utils/api/errors.ts

@afonsojramos afonsojramos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this rollback behaviour end-to-end. I think that the direction is right, and settleNotificationActions is a real improvement on its own over the old Promise.all, which meant a single 403 discarded the outcome of every other notification in a bulk action.

I did reproduce two bugs with throwaway tests on this branch, plus an accessibility regression, are inline below.

Smaller items, not worth their own threads:

  • cancelQueries in onMutate with no optimistic update. All three handlers await queryClient.cancelQueries({ queryKey: notificationsKeys.all }), but the cache write deliberately happens in onSuccess so the exit animation has time to play. That leaves the defensive restore as the snapshot's only consumer. Cancelling an in-flight poll on every click buys little. May be worth snapshotting without cancelling, or a note on why the cancel is wanted.
  • Every NotificationRow subscribes to the whole failures map. useNotifications returns useNotificationActionFailuresStore((s) => s.failures), so a single setFailure re-renders every mounted row. Fine at typical inbox sizes, but a per-id selector would avoid it.
  • Nit: reconcileFailedNotifications re-declares its failed param inline as Array<{ notification; error; rawError }> when FailedNotificationAction[] is already exported from mutations.ts.

CI: the SonarCloud quality gate is red: 3.4% duplication on new code against a 3% gate. The three identical onMutate blocks are the obvious candidate; hoisting them into a shared snapshotNotifications callback should clear it.

I rebased the branch onto main and pushed, the only conflict was an import block in useNotifications.ts (main's Constants import vs this PR's extended stores import).

Signed-off-by: Adam Setch <adam.setch@outlook.com>
@setchy

setchy commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

appreciate all the PR feedback, @afonsojramos.

I've just pushed updates based on OpenSpec+Copilot+Sol's "best" judgement :)

@setchy
setchy requested a review from afonsojramos August 8, 2026 12:08
@sonarqubecloud

sonarqubecloud Bot commented Aug 8, 2026

Copy link
Copy Markdown

@afonsojramos
afonsojramos merged commit cfa12a2 into main Aug 8, 2026
16 checks passed
@afonsojramos
afonsojramos deleted the fix/state-rollback-forbidden-actions branch August 8, 2026 14:52
@github-actions github-actions Bot added this to the Release 7.x.x milestone Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Development

Successfully merging this pull request may close these issues.

2 participants