Skip to content

Conversation

@ameliahsu
Copy link
Member

fixes a bug where X-Hits was turning the total sum of alerts (right column), rather than the total number of groups (rows in the table)

image (2)

@ameliahsu ameliahsu requested a review from a team as a code owner November 14, 2025 20:21
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 14, 2025
if known_hits is not None:
hits = known_hits
elif count_hits:
hits = self.count_hits(max_hits=MAX_HITS_LIMIT)
Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't seem like we're using the max_hits variable from the method signature, should that be used here if set?

Copy link
Member Author

Choose a reason for hiding this comment

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

good callout, updated in 3da9532

)

# Count distinct groups for pagination
group_count = qs.count()
Copy link
Contributor

Choose a reason for hiding this comment

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

just a quick note that this will add another query and may impact performance; i'm guessing we need this to get the total number of results compared to the paginated list though.

elif count_hits:
if max_hits is None:
max_hits = MAX_HITS_LIMIT
hits = self.count_hits(max_hits)
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Inconsistent Paginator Hit Counts

When known_hits is provided, it should be capped by max_hits for consistency with SequencePaginator. Currently, OffsetPaginator uses known_hits directly without applying the max_hits bound. If both parameters are provided together, hits could exceed max_hits, creating inconsistent behavior across paginators.

Fix in Cursor Fix in Web

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

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants