[adityareddy-dev]

Summary

A short search page triggered another query in the same time window. I now skip that query only when the effective query limits rule out truncation. Capped or unknown results keep paging within the window.

Fixes #1403.

How to reproduce

Starting point: A search spanning multiple time windows, with a page limit of 100 and only one matching row in the first window.

On main:

1. Fetch the first page, then request the next page.
2. The next query repeats the first window with OFFSET 1.

On this branch:

1. Repeat the same search with each applicable overflow mode set to throw or its limits disabled.
2. The next query starts the second window with OFFSET 0.
3. When a result, read, timeout or other query limit can truncate the response, a nonempty short page keeps paging within the current window. Unknown settings use the same behavior.

How to test on Vercel preview

N/A, query pagination change with no visual changes.

References

#1403

[github-actions]

Hi @adityareddy-dev, thanks for the pull request!

Before we review code from a first-time contributor, a maintainer vouches for them, and you're not on our list yet. There's nothing else to file and this PR stays open — a maintainer will look at it, usually within a day or two, and vouch for you right here if it's a genuine attempt at the problem.

To make that quick, check the PR description has:

• The issue it addresses (ideally one labelled Accepting PR) and why the change is needed
• Steps to reproduce the problem and verify the fix, starting from https://play.hyperdx.io or yarn app:dev:local connected to the demo server
• Before/after screenshots if anything in the UI changed

More detail in our contributing guide.

[github-actions]

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

• 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
• 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
• Anything about surrounding code, or asking you to widen the change (hoist a helper,
  dedupe with another file, fix other call sites): never fix here, whatever the
  severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

1 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 1 minor

1 posted as inline comment(s) on the changed lines.

---

_{Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.}

[github-actions]

Deep Review

✅ No critical issues found. The core logic is conservative by construction: canSkipShortPage defaults to false whenever settings cannot be read or are inconclusive (empty Map from a privileges failure, caught getSettings rejection, missing leaf settings, or any overflow mode that is not provably non-truncating), and a false flag preserves the prior behavior of continuing to page within the window. A short page is only treated as proof of exhaustion when every covered overflow mode is throw or its limits are 0, so the risk direction favors extra queries over dropped rows. The requestedLimit/no-limit handling in getNextPageParam also removes a prior broken within-window offset path for limitless configs rather than introducing data loss.

🟡 P2 — recommended

• packages/app/src/hooks/useOffsetPaginatedQuery.tsx:275 — The ~60-line overflow-mode inspection (getSetting, cannotTruncate, limitsByMode/leafLimitsByMode, and the final boolean) is inlined in queryFn, so the correctness-critical skip matrix can only be exercised end-to-end through the hook rather than directly.
  • Fix: Extract a pure computeCanSkipShortPage(serverSettings, sourceSettings, queryTimeout, requestedLimit) helper and add table-driven unit tests for each overflow mode and the settings-absent fail-safe.
  • _{maintainability, testing}
• packages/app/src/hooks/useOffsetPaginatedQuery.tsx:296 — The hard-coded overflow-mode→limit tables carry no comment on why this is the complete set or on the intended fail-safe direction; a future ClickHouse truncating setting absent from the table would let canSkipShortPage become true and silently drop rows.
  • Fix: Document which ClickHouse settings/version the tables derive from and state the fail-safe expectation when an unknown truncating limit is active.
  • _{maintainability}

🔵 P3 nitpicks (3)

• packages/app/src/hooks/useOffsetPaginatedQuery.tsx:164 — The predicate lastPage.data.length >= requestedLimit || !lastPage.canSkipShortPage inverts a flag whose name reads as "may skip," making the within-window continuation condition easy to misread.
  • Fix: Add a use-site comment or rename toward consumer semantics such as shortPageProvesExhaustion.
• packages/app/src/hooks/useOffsetPaginatedQuery.tsx:327 — The &&= compound assignment narrows canSkipShortPage with the leaf-settings check in a second stage, obscuring the two-stage data flow.
  • Fix: Compute the base and leaf conditions as separate named booleans and combine them with a single &&.
• packages/app/src/hooks/useOffsetPaginatedQuery.tsx:277 — metadata.getSettings is awaited after renderChartConfig on the query path; although it is cached per connection, the first call per connection serializes behind the render.
  • Fix: Issue the settings fetch concurrently with renderChartConfig and await both together.

---

Reviewers (8): correctness, adversarial, testing, maintainability, performance, reliability, kieran-typescript, previous-comments.

Testing gaps:

• Confirm a positive assertion exists that a short page with non-truncating settings skips straight to the next window (offset 0) rather than issuing a redundant within-window OFFSET query — the merged suite heavily covers the keep-paging (truncatable) direction.
• The limitless-config path (requestedLimit == null) now routes to the next window on a nonempty page; add a case asserting that behavior and the absence of a repeated within-window offset query.

_{No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond}

[adityareddy-dev]

Done.

[vercel]

@adityareddy-dev is attempting to deploy a commit to the ClickHouse Team on Vercel.

A member of the Team first needs to authorize it.

[adityareddy-dev]

Fixed.

[github-actions]

🔵 minor — Empty-setting filter copies joinQuerySettings and can drift from the SQL actually sent

The setting.length && value.length filter repeats the one in joinQuerySettings (packages/common-utils/src/core/utils.ts:1393), which decides which source settings reach the SQL SETTINGS clause. The inference here is only correct if both filters stay identical. Fix: export a small filterQuerySettings (or querySettingsToMap) helper from common-utils and use it in both places.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[github-actions]

🔵 minor — The check for whether query limits can truncate a page is about 60 lines of logic written inline in the queryFn, and it repeats the empty-setting filter from joinQuerySettings

Move this logic into a pure helper next to joinQuerySettings in packages/common-utils/src/core/utils.ts (for example canResultBeTruncated(serverSettings, querySettings, queryTimeout, limit)). The helper should reuse that file's setting.length && value.length filter (utils.ts:1393) so the SETTINGS rendering and this check can't drift apart. Then unit-test it directly instead of through roughly 80 hook renders.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[changeset-bot]

🦋 Changeset detected

Latest commit: 229dbf5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@hyperdx/app	Patch
@hyperdx/api	Patch
@hyperdx/otel-collector	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

🔵 minor — The hook keeps its own copy of how ClickHouse settings take effect, in two places

Move the settings check (lines 275-336) into a pure helper in packages/common-utils/src/core/utils.ts, next to joinQuerySettings. It would take the server settings, source settings, queryTimeout and the requested limit, and return a boolean. Today the inline code copies two pieces of logic. (1) The empty-pair filter at lines 281-283 repeats joinQuerySettings (core/utils.ts:1421). If the copies drift, the decision will be based on settings that differ from the SETTINGS clause actually sent. (2) The max_execution_time precedence (source setting, then the client timeout, then the server value) restates processClickhouseSettings (packages/common-utils/src/clickhouse/index.ts:609-614). A setting added to the client defaults later will not be counted here. Unit-test the helper directly instead of rendering the whole hook for each of the ~90 settings combinations.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}

[adityareddy-dev]

@karl-power can you vouch for me on this one?