Skip to content

Conversation

@afonsojramos
Copy link
Member

@afonsojramos afonsojramos commented Dec 30, 2025

Summary

Adds a new setting to show read notifications alongside unread ones. Fixes #708.

Changes

  • Added showReadNotifications setting (default: disabled)
  • When enabled, fetches both read and unread notifications via all=true API parameter
  • Read notifications display at 50% opacity for visual differentiation
  • "Mark as read" button is hidden on already-read notifications
  • Tooltip includes rate limiting warning for users with many notifications

Why "Mark as read" isn't a toggle

GitHub's REST API only supports one-way transitions: unread → read → done. There's no endpoint to mark a notification as unread, so we can't implement a toggle. See GitHub API docs.

Test plan

  • Toggle setting on → read notifications appear with reduced opacity
  • Toggle setting off → only unread notifications shown (default behavior)
  • "Mark as read" button hidden on read notifications
  • Unit tests pass

@github-actions github-actions bot added the enhancement New feature or enhancement to existing functionality label Dec 30, 2025
@afonsojramos afonsojramos linked an issue Dec 30, 2025 that may be closed by this pull request
@afonsojramos afonsojramos changed the title feat: add 'Show read notifications' setting (#708) feat: add 'Show read notifications' setting Dec 30, 2025
@setchy
Copy link
Member

setchy commented Dec 30, 2025

I've had this on my half-implemented list for a while, with a few raised and then closed PR attemps...

I think we need to wait until the merged/batched api is finished before merging to avoid rate limits.

Additionally, in the previous PRs I raised for this feature, I had changes to the HoverGroup buttons to hide the "mark as read" option on read NotificationRows

@setchy
Copy link
Member

setchy commented Dec 30, 2025

We should also change the look and feel for read NotificationRows - ie: opacity difference, plus as Read State as a Filter Section option for Read and Unread.

Unfortunately the REST API doesn't indicate a difference between read and done notification types

@setchy
Copy link
Member

setchy commented Dec 30, 2025

Unfortunately the REST API doesn't indicate a difference between read and done notification types

To add some further thoughts for discussion, when I last looked into this feature ~2 months or so back that I was 80% convinced to close #708 as not planned since I couldn't see it being that useful alone unless we can find a way to differentiate read + done vs read and undone, or essentially the not done inbox. I timeboxed this but wasn't able to find any attributes within the public REST or GraphQL API endpoints that could help us here...

@afonsojramos
Copy link
Member Author

@setchy, alright, I think I've landed on a good implementation here.

  1. Default is now false (opt-in, preserves existing behavior)
  2. "Mark as read" button is hidden on already-read notifications
  3. Read notifications already render at 50% opacity
  4. Added all=true API param when setting is enabled to actually fetch read notifications
  5. Added rate limiting warning in the tooltip

On the read vs done question:
Done notifications are DELETEd from the API entirely - they never appear in responses. So, the unread boolean cleanly distinguishes read from unread. The concern about differentiating "read + done" vs "read + undone" doesn't apply since done items are gone.

On rate limiting:
The tooltip warns users, and it's opt-in. Happy to consider deferring to when the batched API is ready if you think that's better.

What do you think?

@setchy
Copy link
Member

setchy commented Dec 31, 2025

In addition to the above comments, we should also:

  • Change the Sidebar text when settings.showReadNotifications is enabled to read "XX notifications" instead of "XX unread notifications"
  • Add a new filter section for "Read" and "Unread"

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 1, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
75.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@setchy
Copy link
Member

setchy commented Jan 1, 2026

Unfortunately the REST API doesn't indicate a difference between read and done notification types

To add some further thoughts for discussion, when I last looked into this feature ~2 months or so back that I was 80% convinced to close #708 as not planned since I couldn't see it being that useful alone unless we can find a way to differentiate read + done vs read and undone, or essentially the not done inbox. I timeboxed this but wasn't able to find any attributes within the public REST or GraphQL API endpoints that could help us here...

The attached grab better explains what i poorly trying to explain 😅 - two observed behaviors
1 - When marking a read notification as done, the notification does not get removed it from the list notifications response. The https://github.com/notifications correctly differentiates between read and done, but the list notifications API response doesn't differentiate the two.
2 - when marking a notification as read and settings.fetchReadNotifications is enabled, then the notification row should be set as Opacity.READ and the exit transition animation prevented

Screen.Recording.2026-01-01.at.10.24.48.AM.mov

@afonsojramos
Copy link
Member Author

1 - When marking a read notification as done, the notification does not get removed it from the list notifications response. The github.com/notifications correctly differentiates between read and done, but the list notifications API response doesn't differentiate the two.
2 - when marking a notification as read and settings.fetchReadNotifications is enabled, then the notification row should be set as Opacity.READ and the exit transition animation prevented

Issue number 2 is easily fixable, but issue number one is a doozy... Here are the options I can think of:

  1. Accept the limitation - document it for users
  2. Also suppress done animation when fetchReadNotifications=true (prevents confusing "disappear then reappear")
  3. Track done notifications locally (more complex, scope creep) but likely the best one, long term... What do you think @setchy ?

@setchy
Copy link
Member

setchy commented Jan 5, 2026

Issue number 2 is easily fixable, but issue number one is a doozy... Here are the options I can think of:

1. Accept the limitation - document it for users

2. Also suppress done animation when fetchReadNotifications=true (prevents confusing "disappear then reappear")

3. Track done notifications locally (more complex, scope creep) but likely the best one, long term... What do you think @setchy ?

Agree, that one is a huge doozy 🥴

I think although we could implement option (3) - local done state tracking, it would pretty easily drift from that of the official sources, which would deliver an inconsistent user experience.

My suggestion paths forward would be roughly along these lines

Suggested Path 1 - Accept the limitation and document caveats

  • Rename the setting key and title from Fetch read notifications to something like:
    • Fetch read & done notifications.
    • Fetch all notifications, but this would require us to rephrase the existing option which unpaginates the full inbox list
  • Document the limitation for users via the setting tooltip, explaining that when this new setting is enabled, read, unread and done notifications will be shown and that we aren't able to discern the difference between read and done at this stage
  • For read notifications,
    • Prevent all exit transition animations (open notification, mark as x, etc)
    • Also hide the Mark as done option? I feel this is a less confusing UX then always showing it without any feedback on done vs undone

Suggested Path 2 - Mark as not planned

  • Due to the inability to differentiate read vs done via the known public API endpoints (ie: https://api.github.com/notifications?all=true), leave this feature in the backburner and/or flag as not planned to be implemented for the challenges described above

@setchy
Copy link
Member

setchy commented Jan 5, 2026

I've also updated our #890 to document the ?all=true read vs done challenges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or enhancement to existing functionality

Development

Successfully merging this pull request may close these issues.

Feature request: add a "Show read notifications" setting

3 participants