Skip to content

refactor(app): split DBDashboardPage into focused modules#2233

Open
fleon wants to merge 15 commits into
mainfrom
fleon/refactor-DBDashboardPage
Open

refactor(app): split DBDashboardPage into focused modules#2233
fleon wants to merge 15 commits into
mainfrom
fleon/refactor-DBDashboardPage

Conversation

@fleon
Copy link
Copy Markdown
Contributor

@fleon fleon commented May 7, 2026

Summary

Refactors the large DBDashboardPage module into a DBDashboardPage/ directory with focused components for the tile renderer, heatmap tile, edit modal, container row, header, toolbar, and dashboard grid. The public @/DBDashboardPage import remains unchanged through a barrel export.

Moves pure dashboard layout/grouping helpers into utils.ts and adds targeted tests for helper behavior and dashboard grid actions. This is intended to make future dashboard changes easier to review without changing runtime behavior.

How to test locally or on Vercel

  1. cd packages/app && yarn ci:unit src/DBDashboardPage/__tests__/utils.test.ts src/DBDashboardPage/__tests__/DashboardGrid.test.tsx
  2. cd packages/app && yarn ci:unit
  3. make ci-lint currently reaches unrelated pre-existing source.disabled type errors in packages/api/src/models/source.ts and source UI files; the dashboard refactor files have no IDE linter diagnostics after yarn lint:fix.
  4. make dev-e2e FILE=dashboard was started locally; it was still running after the command was manually backgrounded while the E2E stack was pulling ClickHouse.

References

  • Linear Issue: N/A
  • Related PRs: N/A

Made with Cursor

fleon and others added 13 commits May 7, 2026 15:46
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented May 7, 2026

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment May 11, 2026 4:15pm

Request Review

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 7, 2026

⚠️ No Changeset found

Latest commit: 4738d7a

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

@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label May 7, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

🔴 Tier 4 — Critical

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

Why this tier:

  • Large diff: 5283 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: 12
  • Production lines changed: 5283 (+ 780 in test files, excluded from tier calculation)
  • Branch: fleon/refactor-DBDashboardPage
  • Author: fleon

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
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

PR Review

✅ No critical issues found.

This is a structural refactor of DBDashboardPage.tsx (2464 lines) into a DBDashboardPage/ directory of focused components plus an extracted utils.ts. Public import path is preserved via index.ts.

Spot checks against origin/main:

  • tileToLayoutItem, updateLayout, hasLayoutChanged, downloadObjectAsJson, buildMoveTargets, getTilesByContainerId, getUngroupedTiles, getAlertingTabIdsByContainer, getDashboardTableConnections all match the inline originals semantically.
  • DashboardContainerRow.tsx is a near-verbatim move; the inner shadowed visibleTiles (lines 64–66 vs 95–98) is preserved from the original — outer and inner expressions resolve to the same tile set in all branches (hasTabs / no-tabs / multi-tab), so no behavioral regression.
  • DashboardToolbar, DashboardHeader, EditTileModal, HeatmapTile, DashboardTile, DashboardGrid are extractions without logic changes.
  • New tests (utils.test.ts, DashboardGrid.test.tsx, DashboardTile.test.tsx) cover the extracted helpers and key UI surfaces.

Non-blocking notes (optional follow-up, not required for merge):

  • DashboardContainerRow.tsx:64-70 — the outer visibleTiles is only used to build layoutChangeHandler; consider passing containerTiles + activeTabId to a memo-derived value or removing the shadowing for clarity. Carried over from original code, so not a regression.
  • DBDashboardPage.tsx is still 955 lines — further splitting the orchestration callbacks (collapse/expand URL helpers, container/tile mutations) into a hook could be a nice next step.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

E2E Test Results

All tests passed • 163 passed • 3 skipped • 1202s

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

Tests ran across 4 shards in parallel.

View full report →

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Deep Review

Pure-refactor split of the 2464-line packages/app/src/DBDashboardPage.tsx into a DBDashboardPage/ directory of focused modules. Public import preserved via barrel; behavior intended to be byte-equivalent. A previous Claude review + /just-fix-it cycle already addressed the most mechanical findings (restored displayTypeRequiresSource import, merged duplicate useDashboard, type-only imports, onAddTile signature, expanded DashboardTile/utils tests) -- the previous-comments reviewer verified each of these is in fact present in the current tree. The findings below are either fresh, or items the resolver explicitly skipped that multiple reviewers re-surfaced.

🔴 P0/P1 -- must fix

  • packages/app/src/DBDashboardPage/DBDashboardPage.tsx:557 -- makeOnLayoutChange closes over the render-time dashboard, so an RGL onLayoutChange flushed after a tile delete writes produce(dashboard, ...) against the pre-delete snapshot and resurrects the deleted tile; hasLayoutChanged's newLayout.length !== currentLayout.length short-circuit guarantees the write fires every time.
    • Fix: Hold the live dashboard in a useRef updated by useEffect, read dashboardRef.current inside the layout handler, and filter newLayout against currently-rendered tile ids before producing.
    • adversarial, julik-frontend-races
  • packages/app/src/DBDashboardPage/DBDashboardPage.tsx:474, 506 -- onDuplicateClick and onDeleteClick await confirm(...) and then spread the closure-captured dashboard into setDashboard; any auto-refresh tick, RGL drag, or co-editor mutation that lands during the confirm window is silently clobbered when the user presses Confirm.
    • Fix: Read dashboardRef.current inside the confirm continuation (same ref pattern as the layout-handler fix) instead of the closure variable.
    • adversarial, julik-frontend-races

🟡 P2 -- recommended

  • packages/app/src/DBDashboardPage/DBDashboardPage.tsx -- 955 lines, still ~3x the project's 300-line ceiling per AGENTS.md Key Principles feat: add docker prod build stages and publish prod builds #4 and agent_docs/code_style.md; the file collapses page state, URL-state plumbing, container CRUD, tile CRUD, and the renderTileComponent switch into one module.
    • Fix: Extract useDashboardContainerState (URL collapse/expand/active-tab plumbing, ~344-660) and useDashboardInitialization (saved-query/saved-filter init effect, ~210-265) as a first pass.
    • project-standards, maintainability
  • packages/app/src/DBDashboardPage/DashboardTile.tsx -- 709 lines, also above the 300-line ceiling; mixes queriedConfig derivation, the hover-toolbar memo, the renderChartContent DisplayType dispatcher (~180 lines on its own), and the fullscreen modal in one file.
    • Fix: Extract renderChartContent into a TileChartContent component, split the hover-toolbar JSX into TileHoverToolbar, and re-evaluate.
    • project-standards, maintainability
  • packages/app/src/DBDashboardPage/DashboardContainerRow.tsx:64 -- visibleTiles is computed twice: once on the outer scope (from prop activeTabId, used to bind layoutChangeHandler) and once inside DashboardContainer's render-prop (from currentTabId); the outer-bound handler is also memoized with a visibleTiles dep that's an inline filter(...), so the useMemo never caches. If the inner/outer tab ids ever diverge, hasLayoutChanged compares a 0-tile currentLayout against a multi-tile newLayout and always reports change, firing spurious setDashboard writes on every drag.
    • Fix: Compute visibleTiles once via useMemo([containerTiles, hasTabs, activeTabId]), build the layout handler from the same source the JSX renders, and rename the inner binding so the shadowing can't recur.
    • correctness, maintainability, kieran-typescript
  • packages/app/src/DBDashboardPage/DBDashboardPage.tsx:445 -- renderTileComponent is a useCallback with 14 dependencies (including dashboard and the selectedTileIds Set, both of which churn on unrelated state), and its body rebuilds a fresh filters={[{type, condition: where}, ...filterQueries]} array per tile per render; that array feeds DashboardTile's useEffect([…, filters]), so setQueriedConfig runs for every tile on every parent state change (e.g., shift-click select).
    • Fix: Hoist filters into a useMemo([where, whereLanguage, filterQueries]) at the page level, wrap DashboardTile in React.memo, and read dashboard/selectedTileIds via refs inside the callback so its identity stabilizes.
    • performance, maintainability
  • packages/app/src/DBDashboardPage/DashboardTile.tsx:126 -- every DashboardTile registers a document-level useHotkeys(['f', …]) listener; isFocused is toggled by onMouseOver/onMouseLeave, so the last hovered tile stays "focused" indefinitely. Pressing f while typing into the WHERE/CodeMirror editor (content-editable, not <input>) triggers fullscreen on the stale-focused tile.
    • Fix: Hoist f handling to one page-level useHotkeys that reads a focusedTileId ref and ignore events whose target is content-editable or inside a Mantine input.
    • adversarial, julik-frontend-races
  • packages/app/src/DBDashboardPage/DBDashboardPage.tsx:346 -- urlCollapsedIds, urlExpandedIds, and urlActiveTabs are never pruned when containers/tabs are deleted; deleted-container ids accumulate forever in the URL, and getActiveTabId's fallback to tabs[0] silently masks the desync so the URL never self-heals. Shared URLs leak stale tab/container ids to recipients.
    • Fix: In handleDeleteContainer/handleDeleteTab, strip the deleted id from the relevant nuqs arrays/records; or run a useEffect that prunes them whenever dashboard.containers changes.
  • packages/app/src/DBDashboardPage/DBDashboardPage.tsx:415 -- handleMoveTileToGroup writes tile.containerId/tile.tabId without verifying either still exists in draft.containers; if the move menu is opened in one tab and the target container is deleted in another before the click lands, the tile is moved to a non-existent container and becomes a ghost-tabId ungrouped tile.
    • Fix: Validate containerId/tabId against draft.containers inside the immer mutation; clear containerId and tabId if either is missing.
  • packages/app/src/DBDashboardPage/DBDashboardPage.tsx:681 -- onAddTile computes the new tile position from dashboard.tiles before the previous Add Tile's PATCH lands; rapid clicks produce two tiles at the same (x, y).
    • Fix: Disable the Add Tile button while isSaving is true, or include in-flight tiles in calculateNextTilePosition.
  • packages/app/src/DBDashboardPage/EditTileModal.tsx:37 -- handleClose has three branches (isSaving short-circuit, hasUnsavedChanges confirm flow, the explicit setHasUnsavedChanges(false) reset that prevents double-prompting) and no tests cover any of them; the inline comment explicitly flags the reset as a non-obvious fix.
    • Fix: Add an EditTileModal.test.tsx that mocks useConfirm and asserts: (a) isSaving=true blocks onClose, (b) hasUnsavedChanges=true prompts confirm and only closes on truthy resolve, (c) two close attempts in succession after confirm only trigger one prompt.
🔵 P3 nitpicks (13)
  • packages/app/src/DBDashboardPage/DashboardHeader.tsx:216 -- commented-out <Button>Save</Button> JSX carried verbatim from the deleted monolith.
    • Fix: Delete the dead JSX; track the feature in an issue if it's still wanted.
  • packages/app/src/DBDashboardPage/DashboardGrid.tsx:22 -- const ReactGridLayout = WidthProvider(RGL) is instantiated twice (also in DashboardContainerRow.tsx:11) because the split duplicated the original module-scope binding.
    • Fix: Move the HOC instantiation into a shared module (e.g., ./gridLayout.ts) and import it.
  • packages/app/src/DBDashboardPage/DashboardGrid.tsx:35 -- tileToLayoutItem is a pure import from ./utils.ts prop-drilled through DashboardGrid and DashboardContainerRow despite never being substituted.
    • Fix: Drop the prop and import directly in each leaf.
  • packages/app/src/DBDashboardPage/DBDashboardPage.tsx:121 -- useRef<string>(undefined) narrows current to string under React 19 types even though the initial value is undefined.
    • Fix: useRef<string | undefined>(undefined).
  • packages/app/src/DBDashboardPage/DBDashboardPage.tsx:88 -- useDashboard(dashboardId as string | undefined, …) re-casts a value already declared string | undefined on line 78.
    • Fix: Drop the redundant cast.
  • packages/app/src/DBDashboardPage/DBDashboardPage.tsx:908 -- Add Tile button variant computation falls through to primary when dashboard is undefined (loading), where the pre-refactor monolith used secondary.
    • Fix: Use hasTiles={dashboard?.tiles.length === 0} (matching the original) to preserve the loading-state variant.
  • packages/app/src/DBDashboardPage/DBDashboardPage.tsx:382 -- getActiveTabId silently falls back to tabs[0] when the URL tab id no longer exists; combined with the un-pruned urlActiveTabs (see P2 above), browser back/forward through a co-edited dashboard renders tab 1 while the URL still says tab 2.
    • Fix: Prune urlActiveTabs keys whose containers/tabs no longer exist via a useEffect keyed on dashboard.containers.
  • packages/app/src/DBDashboardPage/__tests__/DashboardTile.test.tsx:136 -- (overrides as any).config defeats the Partial<Tile> typing on makeTile; the cast is unnecessary because overrides.config is already Partial<Tile['config']> | undefined.
    • Fix: Drop the as any and spread overrides.config ?? {}.
  • packages/app/src/DBDashboardPage/__tests__/DashboardGrid.test.tsx:56 -- only canRenderDashboard=false is exercised; the DnD provider + ReactGridLayout + containers.map happy path is uncovered.
    • Fix: Add a test with canRenderDashboard=true, at least one ungrouped tile, and one container, asserting renderTileComponent runs per tile.
  • packages/app/src/DBDashboardPage/DashboardContainerRow.tsx:58 -- the multi-tab visibleTiles filter and the EmptyContainerPlaceholder branch are uncovered (and the shadowing bug above is the exact thing tests should guard).
    • Fix: Add tests for hasTabs=true (only activeTabId tiles flow into the handler) and the empty-active-tab placeholder branch.
  • packages/app/src/DBDashboardPage/HeatmapTile.tsx:41 -- eventDeltasUrl null-guard, separator-picking, and the popover dismiss flow have no tests.
    • Fix: Add tests asserting the popover only opens when source is defined, the link renders when buildEventsSearchUrl returns truthy, and clicking the backdrop resets clickPos.
  • packages/app/src/DBDashboardPage/DashboardHeader.tsx:47 -- isLocalDashboard banner branch, hasTiles Export-menu toggle, and hasSavedQueryAndFilterDefaults Save/Update label are untested.
    • Fix: Add render tests for each branch.
  • packages/app/src/DBDashboardPage/DashboardToolbar.tsx -- Live toggle, manualRefreshCooloff disabling, and form submit dispatch are untested.
    • Fix: Add smoke tests for each.

Reviewers (11): correctness, testing, maintainability, project-standards, kieran-typescript, adversarial, performance, julik-frontend-races, previous-comments, agent-native (no findings -- pure UI refactor; default-export barrel preserves the public surface), learnings-researcher (no findings -- repo has no app-side docs/solutions/ tree yet).

Testing gaps:

  • DBDashboardPage.tsx (955 lines) has no component-level tests; the saved-query init effect, URL collapse-state precedence, and handleMoveTileToGroup recalculation are all untested.
  • No test simulates RGL onLayoutChange firing during a tile delete or during an open confirm dialog -- the exact scenarios for the two P1 stale-closure findings.
  • DashboardContainerRow, DashboardHeader, DashboardToolbar, EditTileModal, HeatmapTile have no dedicated tests.
  • DashboardGrid.test.tsx only exercises the empty state; the happy-path render is uncovered.
  • No regression test for the duplicate-visibleTiles divergence in DashboardContainerRow (drive currentTabId !== activeTabId and assert no bogus setDashboard write).

@teeohhem
Copy link
Copy Markdown
Contributor

teeohhem commented May 7, 2026

/just-fix-it

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Fixed:

  • DashboardTile.tsx:70 — restored displayTypeRequiresSource import from @hyperdx/common-utils/dist/guards, deleted the local stub.
  • DashboardTile tests — new __tests__/DashboardTile.test.tsx covers source-missing/unset (incl. Markdown), alertIndicatorColor for OK/ALERT/silenced/no-alert, filter-warning rendering, and fullscreen toggle (button click + f-no-hover negative).
  • utils tests — added cases for updateLayout mutation, downloadObjectAsJson (mocked DOM helpers), and getDashboardTableConnections (each early return + happy path).
  • DBDashboardPage.tsx:30 — merged duplicate useDashboard import into the existing @/dashboard block.
  • DBDashboardPage.tsx:50 — converted DashboardQueryFormValues/MoveTarget to type-only imports.
  • DashboardContainerRow.tsx:24 — aligned onAddTile signature to (containerId?: string, tabId?: string) => void.

Skipped:

  • P1 f-hotkey positive case — useHotkeys + React 19 useEffectEvent didn't pick up state updates inside synchronous test sequences; covered the same setIsFullscreen surface via the toolbar button click + the no-hover negative.
  • All other P2 findings (visibleTiles shadowing, ref-pattern races, useMemo extractions, React.memo wrapping, O(N²) Map refactors, hook extractions, RGL race fixes) — non-mechanical per rule 2.
  • All P3 nitpicks per rule 3.

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

Labels

review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants