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?

@sonarqubecloud
Copy link


<Checkbox
checked={settings.showReadNotifications}
label="Show read notifications"
Copy link
Member

Choose a reason for hiding this comment

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

"Fetch read notifications" to be consistent with "Fetch only participating" setting?

let passesFilters = true;

// Filter out read notifications if showReadNotifications is disabled
if (!settings.showReadNotifications && !notification.unread) {
Copy link
Member

Choose a reason for hiding this comment

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

This is redundant and can be removed as settings.showReadNotifications controls the API query parameters

Copy link
Member

Choose a reason for hiding this comment

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

In it's place, we should add a new Filter Section for "Read" and "Unread" which runs on the base notifications


<HoverButton
action={actionMarkAsRead}
enabled={!isNotificationRead}
Copy link
Member

Choose a reason for hiding this comment

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

The position of the 3 hover group buttons should be consistent between read and unread notification rows. This currently hides the "mark as read" button and shifts the position of the "mark as done" button

@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"

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