chore: refactor DBSearchPage into smaller files

[karl-power]

Summary

• Split the 2,301-line monolithic packages/app/src/DBSearchPage.tsx into a components/DBSearchPage/ directory of focused files, generated via the refactor-component skill.
• Extracted ~15 sub-components (e.g. SearchTopBar, SearchResultsArea, SearchResultsView, SearchPatternView, SaveSearchModal, SavedSearchHeader, SearchErrorDisplay, SearchQueryRow, SearchSubmitButton, ResumeLiveTailButton, SourceModals, SearchNumRows, SearchResultsCountGroup), plus shared hooks.tsx and utils.ts modules.
• Updated the two import sites (pages/search/index.tsx, pages/search/[savedSearchId].tsx) to consume the new barrel at components/DBSearchPage.
• Moved/expanded test coverage under components/DBSearchPage/__tests__/, adding new unit tests for SearchErrorDisplay, SearchTopBar, and utils.

Why

DBSearchPage.tsx had grown well past the 300-line guideline in AGENTS.md and was painful to navigate, review, and test. Breaking it apart by responsibility makes the search page easier to reason about and gives each piece its own seam for unit testing — without changing runtime behaviour.

How to test locally or on Vercel

1. Test that everything on the search page works as before: filters, sidepanel, saved search etc.
2. Test navigating to and from the search page to saved searches and other pages.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 7a3b3a6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[vercel]

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

Project	Deployment	Actions	Updated (UTC)
hyperdx-oss	Ready	Preview, Comment	May 7, 2026 3:22am

[github-actions]

🔴 Tier 4 — Critical

Touches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD.

Why this tier:

• Large diff: 5592 production lines changed (threshold: 1000)

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats

• Production files changed: 20
• Production lines changed: 5592 (+ 588 in test files, excluded from tier calculation)
• Branch: refactor-DBSearchPage
• Author: karl-power

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

[github-actions]

E2E Test Results

✅ All tests passed • 163 passed • 3 skipped • 1230s

Status	Count
✅ Passed	163
❌ Failed	0
⚠️ Flaky	6
⏭️ Skipped	3

Tests ran across 4 shards in parallel.

View full report →

[github-actions]

PR Review

Pure refactor of DBSearchPage (2,301 LOC → split into 15+ files in components/DBSearchPage/). Imports updated, behavior preserved, new tests added for utils, SearchTopBar, and SearchErrorDisplay.

• ⚠️ hooks.tsx is 954 lines and DBSearchPage.tsx is 694 lines → AGENTS.md sets a 300-line guideline ("Component size: Keep files under 300 lines"). The refactor improves the situation a lot, but hooks.tsx is a single mega-module mixing 7 unrelated hooks (useLiveTailControls, useDirectTraceController, useSavedSearchActions, useChartConfigs, useFormUrlSync, useSourceChangeReset, useQueryErrors). Consider splitting into one file per hook (e.g. hooks/useDirectTraceController.ts) so the new structure actually meets the guideline it cites as the motivation.
• ⚠️ Duplicate getDefaultSourceId test suite in __tests__/utils.test.ts (lines 13-40) and __tests__/hooks.test.tsx (lines 393-445) → Drop the older copy from hooks.test.tsx; that file should only cover hook behavior.
• ⚠️ SearchResultsArea accepts 36 props → Acceptable for a pass-through during this refactor, but worth a follow-up to either co-locate state via context (e.g. live-tail/error state) or group related props (liveTailHandlers, searchState) — the current shape will make the next change to this component painful.
• ℹ️ Minor: useFormUrlSync.reset now writes source: searchedConfig?.source ?? '' where the original wrote ?? undefined. This is actually more correct given SearchConfigSchema requires source: z.string() — flagging only so it's intentional.
• ℹ️ Minor: the // TODO: check edge cases. comment carried over from the original is content-free — drop it or replace with the actual concern.

No security or correctness bugs spotted; the new test coverage and the preserved direct-trace behavior tests are good safety nets.

[teeohhem]

@claude review

[teeohhem]

@claude review

[teeohhem]

@claude review

[github-actions]

Deep Review

🔴 P0/P1 -- must fix

• packages/app/src/components/DBSearchPage/hooks.tsx:779 -- useFormUrlSync resets source: searchedConfig?.source ?? '' (was ?? undefined), so RHF + keepDirtyValues: true treats the empty string as a dirty user-set value and the SOURCE select can silently empty on history-back to a route with no source param.
  • Fix: Restore ?? undefined and add a regression test for back-nav from /search?source=foo to /search.
  • _{adversarial, previous-comments}
• packages/app/src/components/DBSearchPage/hooks.tsx:823 -- useFormUrlSync's second effect dropped sources from its deps array; this works today only because defaultSourceId happens to change reference when sources arrives, and will silently break under any future memoization of getDefaultSourceId.
  • Fix: Restore sources to the deps, or add a comment justifying its intentional exclusion.
• packages/app/src/components/DBSearchPage/DBSearchPage.tsx:85 -- Newly introduced useState-tracked pathname driven by routeChangeStart/routeChangeComplete is a behavior change vs. the original window.location.pathname read; it does not catch popstate or third-party history.pushState, and __tests__/DBSearchPage.test.tsx only registers events.on/off as jest.fn() without firing the captured handlers.
  • Fix: Revert to a direct window.location.pathname read, or add a popstate listener and a test that captures the registered router-event handler and invokes it.
• packages/app/src/components/DBSearchPage/__tests__/hooks.test.tsx:1 -- Six newly extracted hooks (useFormUrlSync, useSourceChangeReset, useLiveTailControls, useSavedSearchActions, useChartConfigs, useQueryErrors) ship with zero direct tests despite the PR's "no behavioural change" claim, leaving the riskiest seams (URL/form sync, live-tail timing, saved-search mutations) without regression coverage.
  • Fix: Add renderHook-based tests for each hook, or extend the integration test to exercise live-tail toggling, saved-search action callbacks, source-switch reset, and post-mutation routing.

🟡 P2 -- recommended

• packages/app/src/components/DBSearchPage/hooks.tsx:1 -- hooks.tsx is 954 LOC bundling 9 unrelated hooks (live-tail, direct-trace, saved-search, chart configs, form/URL sync, source reset, query errors), violating the AGENTS.md 300-line guideline that motivated the refactor and merely relocating the god-module problem.
  • Fix: Split into hooks/{useLiveTailControls,useDirectTraceController,useSavedSearchActions,useChartConfigs,useFormUrlSync,useSourceChangeReset,useQueryErrors}.{ts,tsx} with a hooks/index.ts barrel preserving the import surface.
  • _{maintainability, project-standards, previous-comments}
• packages/app/src/components/DBSearchPage/DBSearchPage.tsx:1 -- The new top-level component is still 694 LOC -- more than 2x the AGENTS.md 300-line ceiling that the PR cites as its motivation.
  • Fix: Extract the prop-construction memoizers (rowTableContext, generateSearchUrl, sourceSchemaPreview) into a useDBSearchPageHandlers hook and lift the JSX into a <DBSearchPageLayout> consumer.
  • _{project-standards, maintainability}
• packages/app/src/components/DBSearchPage/SearchResultsArea.tsx:27 -- SearchResultsArea declares 37 props and forwards ~22 of them verbatim into a near-identical SearchResultsViewProps, so every change requires editing two structurally-coupled inline interfaces in lockstep -- the exact coupling the refactor was meant to remove -- and the {...searchFilters} spread plus inline onCollapse={() => ...} arrows at lines 142/159/196 defeat memo() on DBSearchPageFilters.
  • Fix: Extract a shared SearchResultsViewProps (or a SearchPageContext) that both components consume, memoize useSearchPageFilterState's return, and lift the inline arrows into useCallback.
  • _{maintainability, kieran-typescript, previous-comments}
• packages/app/src/components/DBSearchPage/hooks.tsx:429 -- useSavedSearchActions lists contactSupportNode in handleUpdateTags's deps, but the caller passes <ContactSupportText /> as fresh JSX every render at DBSearchPage.tsx:361, so the callback identity churns into SavedSearchHeader's Tags.onChange on every parent render.
  • Fix: Memoize the JSX in the caller with useMemo, or change the hook API to accept a component reference and instantiate it inside the error branch (matching the original inline pattern).
  • _{adversarial, previous-comments}

🔵 P3 nitpicks (10)

• packages/app/src/components/DBSearchPage/__tests__/hooks.test.tsx:393 -- Duplicate getDefaultSourceId test suite also lives in __tests__/utils.test.ts:13, and the two suites cover slightly different cases (the hooks copy includes the disabled flag) so they will drift independently.
  • Fix: Delete the describe('getDefaultSourceId', …) block at lines 393-445 and keep the disabled-aware version in utils.test.ts.
  • _{testing, previous-comments}
• packages/app/src/components/DBSearchPage/hooks.tsx:770 -- The content-free // TODO: check edge cases. comment was carried over from the original verbatim and conveys nothing.
  • Fix: Drop the comment, or replace it with the actual concern.
• packages/app/src/components/DBSearchPage/hooks.tsx:341 -- useDirectTraceController returns setDirectTraceId, but no caller destructures it.
  • Fix: Remove setDirectTraceId from the returned object.
• packages/app/src/components/DBSearchPage/index.ts:2 -- Exports both default and named { DBSearchPage }; sibling barrels (AppNav/index.ts, DBEditTimeChartForm/index.ts) export default only, and only the new test uses the named form.
  • Fix: Drop the named export and update the test to import the default, or rename the named export to DBSearchPageRaw to make the dual contract explicit.
  • _{maintainability, previous-comments}
• packages/app/src/components/DBSearchPage/DBSearchPage.tsx:245 -- Two useSources() calls (lines 114 and 245) hit the same query; react-query dedupes but the duplication is noise.
  • Fix: Reuse the first result and rename the binding if both names are needed.
• packages/app/src/components/DBSearchPage/SearchSubmitButton.tsx:1 & packages/app/src/components/DBSearchPage/ResumeLiveTailButton.tsx:1 -- 22-26 LOC single-call-site wrappers around one Mantine Button each, with no tests and no reuse.
  • Fix: Inline both at their call sites in SearchQueryRow.tsx and SearchResultsView.tsx and delete the files.
• packages/app/src/components/DBSearchPage/hooks.tsx:928 -- The as Error | ClickHouseQueryError | null cast in useQueryErrors is not load-bearing -- Object.values(queryErrors)[0] ?? null already has that exact type.
  • Fix: Drop the cast.
• packages/app/src/components/DBSearchPage/__tests__/utils.test.ts:1 -- buildChartConfigFromSearchedConfig is exported with two non-trivial branches (early-null when sourceObj is undefined; the orderBy → defaultSearchConfig.orderBy → defaultOrderBy fallback chain) and is only exercised transitively.
  • Fix: Add direct unit tests for the null-source branch and for the order-by fallback chain.
• packages/app/src/components/DBSearchPage/__tests__/hooks.test.tsx:1 & packages/app/src/components/DBSearchPage/__tests__/DBSearchPage.test.tsx:1 -- Both new test files (445 and 377 LOC) exceed the AGENTS.md 300-line ceiling that production files are being held to.
  • Fix: Split each by hook / behavior grouping when the production-file split lands.
• packages/app/src/DBSearchPageAlertModal.tsx:1 -- Still lives at the old top-level path along with __tests__/DBSearchPageQueryKey.test.tsx; the refactor moved everything else into components/DBSearchPage/.
  • Fix: Co-locate DBSearchPageAlertModal.tsx and __tests__/DBSearchPageQueryKey.test.tsx under components/DBSearchPage/ to finish the rename.

---

Reviewers (9): correctness (no findings), testing, maintainability, project-standards, adversarial, kieran-typescript, previous-comments, agent-native (no parity gaps), learnings-researcher (no prior solutions in docs/solutions/).

Testing gaps:

• No back-nav regression test for /search?source=foo → /search that would catch the ?? '' source-clear.
• __tests__/DBSearchPage.test.tsx mocks router.events.on/off as jest.fn() but never invokes the captured handlers, leaving routeChangeStart/routeChangeComplete and the new pathname-derived savedSearchId resync untested.
• No tests for useFormUrlSync URL↔form bidirectional reconciliation, useSourceChangeReset source-change vs. unrelated-config-diff branches, useLiveTailControls interval / refresh-frequency / pause-on-error, useSavedSearchActions post-mutation routing, useChartConfigs memoization keys, or useQueryErrors storage-event clear path.
• SearchResultsArea and SearchResultsView (the largest new components by LOC and prop count) have no dedicated tests covering the analysisMode switch or the queryReady empty-state -- the primary justification for extracting them is missing.
• __tests__/SearchErrorDisplay.test.tsx asserts JSX literal copy (SELECT, Searched For, Query Helper) rather than role/state, so any user-visible copy edit ratchets implementation detail without changing behavior.

[brandon-pereira]

This prop list is a little crazy.. we could ship this but I wonder if some of this could go into context to reduce the prop passing a bit. We would need to watch out for performance regressions if we are storing too much in context though

[brandon-pereira]

Did a test of all the search features and didn't notice any regressions, lots of AI Agent feedback above thats worth checking but IMO this is a worthwhile improvement!