Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filtering on listNotifications #3225

Merged
merged 17 commits into from
Dec 11, 2024
Merged

Filtering on listNotifications #3225

merged 17 commits into from
Dec 11, 2024

Conversation

dholms
Copy link
Collaborator

@dholms dholms commented Dec 10, 2024

Implementation for: #3222

Filter notifications by provided reason. Since this will often dramatically reduce the result set, we also add in some logic for paginating on the server side. Right now, we paginate until one of 3 conditions are met:

  • we find enough posts to reach 1/2 of the provided limit
  • we paginate through 10 pages of notifications
  • we reach the end of the user's notifications

We can adjust the first to numbers if we find the current configuration isn't working as we like

@dholms dholms changed the base branch from main to notif-filter December 10, 2024 22:51
@dholms dholms marked this pull request as ready for review December 10, 2024 22:54
@@ -417,6 +417,84 @@ describe('notification views', () => {
),
).toBe(true)
expect(forSnapshot(notifs.data)).toMatchSnapshot()
await agent.api.app.bsky.notification.putPreferences(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was really weird - i was getting intermittent failures (maybe 1/3 of the time) where updating carol's priority setting would also set alice's to true. I tried for awhile to debug this, but couldn't find the source of it. Verified only one call to mock bsync & that's the only place that I could find where this is updated.

I ended up just adding this call to revert carol's priority setting after the test which is just kinda nice tidiness in the test suite anyway but unblocked this flaky test.

If anyone has ideas on what could be happening plz drop them!

Base automatically changed from notif-filter to main December 11, 2024 19:46
@dholms dholms merged commit 2694c39 into main Dec 11, 2024
10 checks passed
@dholms dholms deleted the notif-filter-impl branch December 11, 2024 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants