Skip to content

feat: allow up to 10,000 filter keys per column in team settings - #3657

Open
knudtty wants to merge 2 commits into
claude/topk-text-index-valuesfrom
claude/filter-keys-fetch-limit
Open

knudtty wants to merge 2 commits into
claude/topk-text-index-valuesfrom
claude/filter-keys-fetch-limit

Conversation

@knudtty

@knudtty knudtty commented Nov 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Increases the max limit of number of keys that can be loaded.

References

  • Linear Issue: Closes HDX-6021

Stack created with GitHub Stacks CLI • Give Feedback 💬

@knudtty
knudtty requested a review from a team as a code owner November 13, 2026 20:50
@knudtty
knudtty added this pull request to stack #3643 November 13, 2026 20:50
@vercel

vercel Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
hyperdx-oss Ready Ready Preview Nov 13, 2026 9:12pm UTC
hyperdx-storybook Ready Ready Preview Nov 13, 2026 9:12pm UTC

Request Review

@changeset-bot

changeset-bot Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: accc11e

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

This PR includes changesets to release 4 packages
Name Type
@hyperdx/app Patch
@hyperdx/common-utils 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

github-actions Bot commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

E2E Test Results

✅ All tests passed • 467 passed • 1 skipped • 1174s

Status Count
✅ Passed 467
❌ Failed 0
⚠️ Flaky 1
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label Nov 13, 2026
@github-actions

github-actions Bot commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

  • Standard feature/fix — introduces new logic or modifies core functionality

Additional context: spans packages but only 15 prod lines — under the 100-line cross-layer bar

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats
  • Production files changed: 3
  • Production lines changed: 15 (+ 29 in test files, excluded from tier calculation)
  • Branch: claude/filter-keys-fetch-limit
  • Author: knudtty

To override this classification, remove the review/tier-2 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@github-actions

Copy link
Copy Markdown
Contributor

Duplicate code analysis

5 new clones introduced by this PR.

  • packages/api/src/models/source.ts:179-205 ↔ packages/api/src/models/source.ts:235-261 — 27 lines
  • packages/common-utils/src/core/metadata.ts:1291-1315 ↔ packages/common-utils/src/core/metadata.ts:1339-1360 — 25 lines
  • packages/common-utils/src/core/metadata.ts:1813-1822 ↔ packages/common-utils/src/core/metadata.ts:1942-1951 — 10 lines
  • packages/common-utils/src/core/metadata.ts:1822-1835 ↔ packages/common-utils/src/core/metadata.ts:1951-1965 — 14 lines
  • packages/common-utils/src/types.ts:2769-2783 ↔ packages/common-utils/src/types.ts:2816-2830 — 15 lines

If one of these duplicates a helper that already exists, import it instead
of redefining it. If the duplication is deliberate, ignore this comment —
the check is advisory and never fails the build.


jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 108 in total (1.17%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

@github-actions

github-actions Bot commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

PR Review

✅ No issues found.

@github-actions

github-actions Bot commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

This is a small, well-tested change: it raises the filterKeysFetchLimit team-setting UI cap from 1000 to 10000 and threads a new optional maxKeys parameter through Metadata.getAllFields into both getMapKeys and getJSONKeys, flooring discovery at DEFAULT_MAX_KEYS (1000) so a low setting never shrinks field-picker results. The maxKeys value is correctly added to the react-query queryKey, getMapKeys/getJSONKeys already validate the value via inlineNonNegativeInt before it reaches the ClickHouse LIMIT, and the happy-path behavior for the default (100) setting is unchanged.

🟡 P2 — recommended

  • packages/common-utils/src/__tests__/metadata.test.ts:3826 — the new maxKeys threading is asserted only for the Map-column path (getMapKeys); the JSON-column branch in getAllFields that now also forwards maxKeys to getJSONKeys has no equivalent assertion.
    • Fix: add a test that drives a JSON column through getAllFields and asserts getJSONKeys is called with the expected maxKeys.
    • testing
  • packages/app/src/hooks/useMetadata.tsx:526 — the same team setting now scales to 10,000 and feeds useMultipleGetKeyValues, where each retained key per column dispatches value lookups chunked by GET_ALL_KEY_VALUES_CHUNK_SIZE (100), so a team opting into a high value can fan out roughly 100 chunk operations per Map column per source against ClickHouse.
    • Fix: confirm the intended ceiling is acceptable for value-lookup load and consider a server-side bound on filterKeysFetchLimit rather than relying only on the UI max.
    • performance
🔵 P3 nitpicks (2)
  • packages/app/src/hooks/useMetadata.tsx:285 — the setting carries two different semantics: discovery floors it at 1,000 (Math.max(..., DEFAULT_MAX_KEYS)) while useMultipleGetKeyValues and DBSearchPageFilters/hooks.ts use the raw value, so any configured value at or below 1,000 has no effect on key discovery despite the field defaulting to 100.
    • Fix: extend the inline comment or the setting's tooltip to state that values at or below 1,000 do not change key discovery, only value lookups.
  • packages/common-utils/src/types.ts:2590 — filterKeysFetchLimit is declared z.number().optional()/.nullish() with no upper bound, so the new 10,000 UI cap is the only guard and can be exceeded via the API.
    • Fix: add a .max(10000) (or the agreed ceiling) to the schema so the bound is enforced server-side as well as in the form.

Reviewers (5): correctness, testing, maintainability, performance, typescript.

Testing gaps: getJSONKeys maxKeys threading is not directly asserted; the max={10000} UI boundary and the discovery floor for settings above 1,000 are exercised only indirectly.

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

This branch was successfully deployed

2 active deployments
Preview – hyperdx-oss — accc11ea Deployed Nov 13, 2026 by vercel[bot]
Preview – hyperdx-storybook — accc11ea Deployed Nov 13, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-2 Low risk — AI review + quick human skim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant