Skip to content

chore: adopt React 19 context/ref APIs and enforce via ESLint - #2911

Merged
kodiakhq[bot] merged 3 commits into
mainfrom
brandon/brandon-eslint-react19-context-ref
Aug 17, 2026
Merged

chore: adopt React 19 context/ref APIs and enforce via ESLint#2911
kodiakhq[bot] merged 3 commits into
mainfrom
brandon/brandon-eslint-react19-context-ref

Conversation

@brandon-pereira

Copy link
Copy Markdown
Member

What

Migrates the app to React 19's context and ref conventions, then promotes the corresponding @eslint-react rules to error so they can't regress.

Rule Sites Migration
no-context-provider 17 <XContext.Provider><XContext>
no-use-context 16 useContext(X) / React.useContext(X)use(X) / React.use(X)
no-forward-ref 3 forwardRef((props, ref) => …)({ …props, ref }) => … (ref-as-prop)

The three forwardRef conversions are Tile (DBDashboardPage), EventRow (SessionEventList), and ValueRenderer (HyperJson).

Why

React 19 is in place (react@^19.2.3). These are the idiomatic replacements for the legacy context/ref APIs, and enforcing them as error stops the old patterns from creeping back in. This continues the lint-warning cleanup started in #2903.

Enforcement

  • @eslint-react/no-context-provider, no-use-context, no-forward-referror in packages/app/eslint.config.mjs
  • App --max-warnings lowered 601 → 565

Verification

  • yarn lint (app) — pass (0 errors; 3 rules now enforced as error)
  • tsc --noEmit (app) — clean
  • yarn knip — clean (removed forwardRef import orphaned nothing)
  • yarn ci:unit173 suites / 2820 tests pass (behavioral migrations verified, including the 3 touched test files)

Behavior is unchanged. Changeset included (@hyperdx/app patch).

@vercel

vercel Bot commented Aug 14, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview Aug 17, 2026 8:54pm
hyperdx-storybook Ready Ready Preview Aug 17, 2026 8:54pm

Request Review

@changeset-bot

changeset-bot Bot commented Aug 14, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 17681bc

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

@greptile-apps

greptile-apps Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adopts React 19 context and ref conventions throughout the app and enforces those conventions through ESLint. It also restores dashboard release markers that were temporarily lost during the migration.

  • Replaces legacy context providers and useContext calls with React 19 APIs.
  • Converts three forwardRef components to ref-as-prop components.
  • Promotes the corresponding ESLint rules to errors and lowers the warning ceiling.
  • Includes a patch changeset for @hyperdx/app.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/app/src/DBDashboardPage.tsx Converts Tile to React 19 ref-as-prop semantics while preserving dashboard behavior and restoring release-annotation querying, merging, rendering, and toggle wiring.
packages/app/src/SessionEventList.tsx Converts EventRow from forwardRef to a typed ref prop and retains attachment to the measured row element.
packages/app/src/components/HyperJson.tsx Converts the memoized ValueRenderer to a typed ref prop while preserving ref attachment across rendered value variants.
packages/app/eslint.config.mjs Enforces the migrated React 19 context and ref conventions as lint errors.
packages/app/package.json Lowers the app lint warning ceiling to match the warnings removed by this migration.

Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile

Migrate the app to React 19's context and ref conventions, then promote the
corresponding lint rules to error so they can't regress:

- Render <Context> directly instead of <Context.Provider> (17 sites)
- Use the `use` hook instead of `useContext` (16 sites)
- Pass `ref` as a regular prop instead of wrapping in forwardRef (3 sites)

Promotes @eslint-react/no-context-provider, no-use-context, and no-forward-ref
to error, and lowers the app --max-warnings ceiling from 601 to 565.

Behavior is unchanged: 2820 unit tests pass, tsc and knip are clean.
@brandon-pereira
brandon-pereira force-pushed the brandon/brandon-eslint-react19-context-ref branch from f789353 to 6865069 Compare August 14, 2026 15:34
@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label Aug 14, 2026
@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI.

Why this tier:

  • Large diff: 2313 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: 27
  • Production lines changed: 2313 (+ 12 in test files, excluded from tier calculation)
  • Branch: brandon/brandon-eslint-react19-context-ref
  • Author: brandon-pereira

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

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

This is a behavior-preserving React 19 idiom migration. All 7 contexts are genuine createContext objects, every use() swap is an unconditional top-level call (no useContextuse semantic divergence), and all three forwardRef→ref-as-prop conversions attach ref to the same DOM node as before. The ref-consuming integrations were traced and hold under React 19 ref-as-prop bridging: react-grid-layout injects ref into Tile via cloneElement, TanStack virtualizer wires EventRow's ref to measureElement, and ValueRenderer's React.memo still short-circuits because its only callers pass a stable useRef. tsc, lint, and the three touched test files are consistent no-op provider-syntax swaps with no weakened assertions.

🔵 P3 nitpicks (1)
  • packages/app/src/DBDashboardPage.tsx:446Tile types its ref-as-prop as ref?: ForwardedRef<HTMLDivElement>, while EventRow and ValueRenderer use ref?: React.Ref<...>; ForwardedRef is the legacy forwardRef-callback type and is strictly narrower (its callback member excludes React 19 cleanup-returning ref callbacks), so the convention is inconsistent across the three sites migrated in the same diff.
    • Fix: Retype Tile's ref prop as ref?: React.Ref<HTMLDivElement> and drop the now-stale ForwardedRef import for parity with the other two conversions.
    • kieran-typescript, maintainability

Reviewers (5): correctness, kieran-typescript, julik-frontend-races, testing, maintainability.

Testing gaps: The three forwardRef→ref-as-prop conversions are the only behavior-affecting part of this change, and each ref consumer fails silently if attachment regresses, yet none is covered by a test:

  • Tile (DBDashboardPage.tsx) — react-grid-layout tile positioning/drag/resize measurement; no test asserts ref attachment.
  • EventRow (SessionEventList.tsx) — TanStack measureElement row sizing; no test file exists for SessionEventList at all.
  • ValueRenderer (HyperJson.tsx) — valueRef-driven selectstart/mouseup selection listeners; HyperJson.test.tsx never exercises the ref path.

@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 300 passed • 1 skipped • 1172s

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

Tests ran across 4 shards in parallel.

View full report →

The React 19 context/ref migration accidentally dropped the dashboard
release-markers feature while rewriting DBDashboardPage.tsx, contradicting
its "behavior unchanged" claim. A user opening the dashboard menu lost the
"Show/Hide release markers" toggle, and a shared URL carrying
releaseMarkers=true silently rendered no release annotations.

Restore the wiring on top of the migrated file (without reverting the
migration):
- useReleaseAnnotations + mergeAnnotations imports and the releaseMarkers
  URL query state
- the showReleaseAnnotations Tile prop, the per-tile releaseAnnotations
  query, and the merged annotations passed to the chart
- the toggle-release-annotations menu item

Also restores the tileCanDrawAnnotations guard on alertAnnotations, which
the migration had likewise dropped. This re-gives useReleaseAnnotations and
mergeAnnotations a production consumer, so their tests are no longer
orphaned.
kodiakhq Bot pushed a commit that referenced this pull request Aug 14, 2026
…2912)

## What

Renames the 10 `useRef` values flagged by `@eslint-react/naming-convention/ref-name` to end in `Ref`, then promotes the rule to `error` so the convention holds going forward.

| File | Refs renamed |
|---|---|
| `DOMPlayer.tsx` | `wrapper`, `playerContainer`, `replayer`, `initialEvents`, `setLastEventTsLoaded` → `*Ref` |
| `DBEditTimeChartForm/EditTimeChartForm.tsx` | `subFormDirty`, `autoRunFired` → `*Ref` |
| `hooks/useMetadata.tsx` | `settingsApplied` → `settingsAppliedRef` |
| `sessions.ts` | `lastAbortController` → `lastAbortControllerRef` |
| `utils.ts` | `observer` → `observerInstanceRef` |

## Why

Consistent `Ref` naming makes ref values obvious at a glance. This is the next mechanical batch in the ongoing lint-warning cleanup (follows #2903 and #2911).

## Notes

- In `utils.ts`, the intersection-observer hook already had an `observerRef` **callback** ref, so the `useRef` was renamed to `observerInstanceRef` to avoid a redeclaration collision (rather than the mechanical `observerRef`).
- `DOMPlayer.tsx`'s diff is larger only because `replayer` had 39 usages — all pure renames. `styles.playerContainer` (a CSS class) was deliberately left untouched.

## Enforcement

- `@eslint-react/naming-convention/ref-name` → **`error`**
- App `--max-warnings` lowered **601 → 590**

## Verification

- `yarn lint` (app) — pass (0 errors; rule now enforced as error)
- `tsc --noEmit` (app) — clean
- `yarn knip` — clean
- `yarn ci:unit` — **174 suites / 2898 tests pass**

Behavior is unchanged. Changeset included (`@hyperdx/app` patch).
@brandon-pereira
brandon-pereira requested review from a team, knudtty and wrn14897 and removed request for a team and knudtty August 14, 2026 18:37
kodiakhq Bot pushed a commit that referenced this pull request Aug 14, 2026
…ules (#2913)

## What

Clears the last small batch of api ESLint warnings and promotes their rules to `error`.

| Rule | Count | Resolution |
|---|---|---|
| `@typescript-eslint/no-namespace` | 2 | Merge the duplicate Express `declare global` namespace blocks; scoped disable (pattern is required) |
| `@typescript-eslint/no-empty-object-type` | 1 | Scoped disable on `interface User extends UserDocument {}` (required for Express `User` augmentation) |
| `n/no-process-exit` | 5 | Config override: rule `off` for the two process entry-point files |

## Why

These are all **legitimate patterns the linter flags**, not real issues:

- **Express type augmentation** (`namespace Express { interface User … }`) is the documented way to extend Express's types — interface merging is required, and a type alias / non-empty interface can't do it. I merged the two duplicate `namespace Express` blocks into one for tidiness and disabled the two rules on that single augmentation block with an explanatory comment.
- **`process.exit()`** in `src/index.ts` (startup failure, last-resort `uncaughtException`/`unhandledRejection` handlers) and `src/tasks/index.ts` (CLI task completion/failure) is the intended behavior. Rather than 5 inline disables, the rule is scoped `off` for just those two entry-point files — so it still guards against stray `process.exit()` elsewhere in the api.

Promoting all three to `error` turns these into guardrails: new violations outside the sanctioned spots now fail lint. This is the last of the small mechanical/one-off batches (follows #2903, #2911, #2912).

## Verification

- `yarn lint` (api) — pass (0 errors; 3 rules now enforced as error)
- `tsc --noEmit` (api) — clean (Express augmentation still resolves after the merge)
- `yarn knip` — clean
- api `--max-warnings` lowered **311 → 302**

Behavior is unchanged. Changeset included (`@hyperdx/api` patch).
pulpdrew
pulpdrew previously approved these changes Aug 17, 2026

@pulpdrew pulpdrew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

wrn14897
wrn14897 previously approved these changes Aug 17, 2026
…t-react19-context-ref

# Conflicts:
#	packages/app/eslint.config.mjs
#	packages/app/package.json
#	packages/app/src/DBDashboardPage.tsx
@kodiakhq
kodiakhq Bot merged commit 905d194 into main Aug 17, 2026
27 checks passed
@kodiakhq
kodiakhq Bot deleted the brandon/brandon-eslint-react19-context-ref branch August 17, 2026 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants