[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 change is conservative by construction: canSkipShortPage is only true when server/source settings prove no truncation is possible, and every failure or unknown-settings path (undefined settings, empty-Map permission error, non-numeric values, partial leaf settings, requestedLimit null/0) resolves to keep-paging — so no window is skipped prematurely and no rows are dropped. The redundant getSettings() round-trip is cached per connection, so the optimization is a net query reduction. Findings below are recommendations and nits.

🟡 P2 -- recommended

• packages/app/src/hooks/useOffsetPaginatedQuery.tsx:297 -- The soundness of canSkipShortPage depends on limitsByMode/leafLimitsByMode enumerating every truncating ClickHouse overflow mode; a future mode that defaults to break with a nonzero limit would be omitted, letting a truncated short page read as window exhaustion and silently drop result pages.
  • Fix: Add a provenance comment naming the ClickHouse version these tables derive from and a test that fails when system.settings exposes an overflow mode not covered here.
  • _{maintainability}
• packages/app/src/hooks/useOffsetPaginatedQuery.tsx:277 -- The new metadata.getSettings() call on the first-page path is awaited before the query's AbortController is created and is passed no abort signal, so a hung system.settings fetch is not cancelled by the query timeout that protects the rest of queryFn.
  • Fix: Pass the request abort signal (or a bounded timeout) into getSettings, or move the fetch under the existing AbortController.
  • _reliability

🔵 P3 nitpicks (3)

• packages/app/src/hooks/useOffsetPaginatedQuery.tsx:275 -- The ~60-line canSkipShortPage settings-resolution block is inlined in the already-large queryFn despite being pure in its inputs, so the overflow-mode logic can only be exercised through the full hook.
  • Fix: Extract to a named helper such as computeCanSkipShortPage({ metadata, config, source, queryTimeout }) and unit-test it directly against a settings map.
• packages/app/src/hooks/useOffsetPaginatedQuery.tsx:314 -- Local const limit holds the ClickHouse server-side limit setting but sits adjacent to limit >= (config.limit?.limit ?? 0), placing two different "limit" concepts on one line.
  • Fix: Rename the local to serverLimitSetting.
• packages/app/src/hooks/useOffsetPaginatedQuery.tsx:522 -- The non-streaming return path now carries canSkipShortPage, but no test fetches past a short page on that path to assert the resulting offset/window, so dropping the field there would go uncaught.
  • Fix: Extend a refetch test to fetchNextPage after a short page and assert the renderChartConfig offset and windowIndex.

---

Reviewers (8): testing, maintainability, project-standards, performance, reliability, previous-comments, learnings-researcher, agent-native. (The correctness, adversarial, and TypeScript reviewers did not return before synthesis; the orchestrator performed independent correctness, data-loss, and type-safety analysis of the diff in their place and found no additional defects — notably the parseToNumber-undefined/limit != null handling and the empty-Map permission path are both correct.)

Testing gaps:

• No test exercises the getSettings() non-permission rejection path (undefined → keep paging) or the permission-error empty-Map path (confirming it does not wrongly set canSkipShortPage=true).
• The sourceSettings empty setting-name/value filter branch is never exercised by a test.
• No test asserts the canSkipShortPage effect on the non-streaming (first-page refetch) return path.

_{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}