test(app): replace unsafe mock assertions with shoehorn - #2980
Conversation
|
|
@k4its1t is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
external
needs-vouch
|
Hi @k4its1t, thanks for the pull request! Before we review code from a first-time contributor we ask that a maintainer vouches for you, and you're not on our list yet. This PR stays open — it just isn't in the review queue until someone vouches. To get vouched, open an issue saying hello and what you're working on: https://github.com/hyperdxio/hyperdx/issues/new?template=introduce-yourself.md A maintainer will usually reply within a day or two, and then this PR gets picked up as normal. More detail in our contributing guide. |
Greptile SummaryThe PR replaces unsafe assertions in three app test suites with complete typed source fixtures, partial query-hook helpers, and enum values.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/app/src/components/tests/DBRowSidePanel.viewTraceTimeFilter.test.tsx | Replaces partial log-source assertions with the existing complete factory while preserving trace and timestamp overrides. |
| packages/app/src/components/tests/DBTimeChart.test.tsx | Uses complete log-source fixtures, typed partial hook results, and the DisplayType.Line enum without changing tested production behavior. |
| packages/app/src/components/tests/MetricTableModelForm.test.tsx | Defines a complete metric fixture and uses the existing log-source factory for the source-kind transition scenario. |
| packages/app/package.json | Adds the test-only shoehorn helper as a development dependency. |
| scripts/ci/ratchet-baseline.json | Lowers app assertion and lint-suppression ceilings to counts matching the current tree. |
| yarn.lock | Records the shoehorn dependency and updates the app workspace resolution consistently. |
Reviews (9): Last reviewed commit: "Merge main and reconcile app ratchet bas..." | Re-trigger Greptile
Deep Review✅ No critical issues found. This is a narrowly-scoped, test-only refactor: it swaps unsafe partial-mock assertions (
🔵 P3 nitpicks (1)
Reviewers (6): correctness, testing, maintainability, project-standards, kieran-typescript, previous-comments. |
|
/vouch @pttydou wants to improve FE type safety |
Triggered by [comment](#2980 (comment)) from @jordan-simonovski. Vouch: @pttydou
…gate (#3024) ## Summary `claude-code-review.yml` has been disabled since **2026-05-20**, so this is a rebuild rather than a tweak. It replaces the prompt, moves it into a versioned file, and changes how findings reach the PR. **The prompt was selected by measurement, not taste.** I built an offline harness that scores reviewer prompts against a frozen dataset of **122 hyperdx PRs where a human left a substantive inline review comment**, using those comments as ground truth. On the held-out test split (Opus, 49 PRs / 87 gold items): | prompt | recall | cost/PR | agents | | --- | --- | --- | --- | | lifted-budget variant of the old prompt | 31% | $2.86 | 1 | | **this prompt** | **40%** | **$3.11** | **1** | | `deep-review` (currently in production) | ~22–33% | — | 6–13 | On the larger shared slice this prompt and `deep-review` are within noise of each other, so **the claim is parity-or-better at one agent instead of six-to-thirteen — not a 4× improvement.** I'd rather understate that than have it quoted back later. ### What changes for reviewers - **Findings post as inline comments on the changed lines.** 82% of findings anchor to a diff line (measured); the rest reference files the diff never touches and fall back to the sticky summary rather than being dropped — that class is a large share of the useful output. - **One sticky summary**, updated in place, with counts and anything unanchored. - **Repeat comments are suppressed across pushes** via a hidden per-finding fingerprint keyed on path + normalized title, so a reworded body doesn't repost. - **Minor findings are folded behind `<details>`, not filtered.** On the dataset, minor-severity findings carry **29–57% of everything a human independently flagged** — filtering them would cost roughly half the recall. Severity is used for ordering only. ### Cost and correctness controls - **Review gate.** Hashes the effective diff (`merge-base..HEAD`) and the prompt; skips when both are unchanged. `synchronize` fires on every "Update branch" merge, which advances the merge-base while leaving the diff byte-identical — without this, a large share of runs are pure waste at ~$3–5/PR. Same idea as `deep-review.yml`. - **Fail-open.** The state marker is only stamped when the run produced parseable output; otherwise the next push retries instead of the gate caching a zero-finding review against that diff forever. - **`concurrency` + cancel-in-progress**, so two quick pushes can't race the sticky comment or pay twice. - **Model pinned to `opus`.** This prompt gained ~13 points from Opus where the old one gained ~5 — the extra passes only pay off at that tier, and an action-default change shouldn't silently swap the reviewer. ### Tool grants Follows the pattern already set in `deep-review.yml`: a read-only `git`/`gh` prefix allowlist. Withheld deliberately, since this job checks out fork-authored code and holds secrets: - **`gh api`** — accepts `--method POST`, and prefix allowlists can't constrain flags (your own note in `deep-review.yml`). Prior review threads are materialized in trusted shell instead, fenced and capped, and the prompt instructs the reviewer not to re-report them. - **bare `Bash`** — would be arbitrary code execution on PR-author-controlled build scripts next to `ANTHROPIC_API_KEY`. - **`WebFetch`/`WebSearch`/`curl`/`wget`** — data egress from a job holding secrets. ### Testing `.github/scripts/review-comments.cjs` holds the comment-routing logic so it's testable without triggering a PR event. 11 tests, run by this workflow before the review step (same pattern as `pr-triage.yml`). They pin the two silent failure modes — a wrong line map, and a fingerprint that reposts — plus the fail-open contract, so nobody "tidies" the marker format and quietly disables the gate. ### Dry run on live PRs Ran against #2992, #2981, #2980 (none in the eval set) without posting: 17 findings, 14 inline, 3 in summary, $10.86. Full output on request. The standout was a `critical` on #2981 that neither `greptile` nor `deep-review` reported — quoting every builder column makes `ORDER BY "count()"` unresolvable for unaliased projections. ## Merging this does not turn it on The workflow is `disabled_manually`; merging the file doesn't change that. Enabling is a separate, deliberate step: ``` gh api -X PUT repos/hyperdxio/hyperdx/actions/workflows/claude-code-review.yml/enable ``` I'd suggest enabling it **alongside** `deep-review` for a couple of weeks (two comments per PR, distinct markers) and comparing on live PRs before deciding whether this replaces it. If it does, expect a net spend *decrease* — one agent instead of 6–13. ## Known gaps, stated plainly - **Two changes here are unevaluated.** The harness measured the prompt with the diff *inlined* and with `Read/Grep/Glob` only. CI reads the diff from a file, and grants read-only `git`/`gh` plus materialized prior comments. Those should help, but the 40% figure doesn't cover them. Prior comments in particular are *unevaluable* in the harness — on the eval PRs those comments **are** the gold set. - **Cost may run above $3.11/PR.** The dry run averaged $3.62 on mid-sized PRs. If the bill looks wrong, the natural lever is gating on the existing `review/tier-N` labels and only running this on tier-3/4. - **One measured gap.** On the dry-run PRs, both `greptile` and `deep-review` independently caught failures involving *data persisted before the change* (stale saved sort strings) and this prompt caught neither. It has no pass for that class. That's the next thing I'd add. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
Merged the latest The only textual conflict was
The upstream package baselines, auto-merged Validation:
The existing |
|
Conflict resolved in merge commit 1e85e83, without rebasing or force-pushing. The sole textual conflict was the ratchet baseline: upstream @source and other package changes are retained, with app thresholds set to measured counts (204 / 0 / 143 / 0). Frozen install, lint, type checking, ratchet, all unit suites and commit hooks passed; full details are in the updated description. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | ||
| const ROOT_SOURCE = { | ||
| const ROOT_SOURCE = fromPartial<TSource>({ |
There was a problem hiding this comment.
🔵 minor — Existing typed source fixture factory not reused; shoehorn moves the unsafe assertion into node_modules rather than removing it
packages/app/src/llm/__fixtures__/sources.ts already exports makeLogSource(overrides: Partial<TLogSource>): TLogSource and makeTraceSource(...) — cast-free, fully-typed factories used by six existing test files. ROOT_SOURCE here is exactly a log source, so makeLogSource({ id: 'log-src', traceSourceId: 'trace-src', timestampValueExpression: 'Timestamp', resourceAttributesExpression: 'ResourceAttributes' }) type-checks every field for real, with no assertion anywhere. That matters because fromPartial is <T>(t: DeepPartial<T>) => t as T: the object still isn't a valid TSource, the assertion just now lives in a dependency where neither @typescript-eslint/no-unsafe-type-assertion (set to warn at packages/app/eslint.config.mjs:165 — the rule those deleted eslint-disable comments were suppressing) nor scripts/ci/ratchet.mjs can see it, which is what drives the baseline from 215→204 and 146→143. Prefer promoting makeLogSource/makeTraceSource out of src/llm/__fixtures__ into a shared test-fixtures module and adding a makeMetricSource for MetricTableModelForm.test.tsx's SAVED_SOURCE (packages/app/src/components/tests/ContextFilterPills.test.ts:13 has a third hand-rolled copy of the same fixture, so a shared home is already overdue) — the DRY rule in the conventions is marked REQUIRED. If shoehorn is kept anyway, the fixtures it wraps are still unvalidated, so say so rather than counting it as a ratchet improvement.
There was a problem hiding this comment.
Keeping the shared-factory migration out of this conflict-resolution update: #2852 explicitly requests shoehorn for these three partial-mock suites. I agree that fromPartial is not runtime validation and does not construct a complete source; it checks supplied fields while the test deliberately supplies only those it needs. The ratchet decrease measures explicit escape-hatch occurrences, not proof that every fixture is complete. I am narrowing the two log fixtures to TLogSource as suggested in the other comment, without moving fixtures across modules or adding a factory family.
| ...SAVED_SOURCE, | ||
| const { metricTables: _metricTables, ...savedSourceWithoutMetricTables } = | ||
| SAVED_SOURCE; | ||
| savedSource = fromPartial<TSource>({ |
There was a problem hiding this comment.
🔵 minor — fromPartial<TSource> over the discriminated union checks essentially nothing
TSource is a discriminated union (TLogSource | TTraceSource | TMetricSource | ...). DeepPartial<T> distributes over unions, so DeepPartial<TSource> makes every field of every kind optional, and TypeScript's excess-property check against a union accepts any property present in any constituent. fromPartial<TSource>({ kind: SourceKind.Log, metricTables: {}, spanIdExpression: 'x' }) compiles clean — the fixture is only checked to be an object. Use the narrow member type, which is already exported (TLogSource, packages/common-utils/src/types.ts:2404) and which this same file uses correctly for TMetricSource at line 130: fromPartial<TLogSource> here, and fromPartial<TLogSource> for ROOT_SOURCE at DBRowSidePanel.viewTraceTimeFilter.test.tsx:161. With TLogSource the metricTables key genuinely has to be stripped, so the destructure at line 244 becomes enforced by the compiler instead of being a convention the reader has to trust.
There was a problem hiding this comment.
Addressed in 1e2f06b: both log fixtures now use fromPartial, retaining TSource only where the harness genuinely handles multiple source kinds. No fixture factory refactor or runtime behavior change. The two affected suites passed 16 tests; yarn lint:fix, make ci-lint, make ci-unit (app 227 suites / 3,699 tests), lint-staged and knip all passed after the change.
PR Review2 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 2 minor 1 posted as inline comment(s) on the changed lines. 1 listed below. Findings outside the changed lines1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. |
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | ||
| const ROOT_SOURCE = { | ||
| const ROOT_SOURCE = fromPartial<TLogSource>({ |
There was a problem hiding this comment.
🟠 major — New shoehorn dependency duplicates the existing log/trace source fixture factories
packages/app/src/llm/__fixtures__/sources.ts:36 already exports makeLogSource(overrides: Partial<TLogSource>): TLogSource (and makeTraceSource), which builds a complete source with no cast and no third-party helper; it is already imported from tests via @/llm/__fixtures__/sources (e.g. packages/app/src/llm/__tests__/ErrorsTab.test.tsx:3), and packages/app/src/components/__tests__/ContextFilterPills.test.ts:13 is a third hand-rolled copy of the same operation. Per the REQUIRED "Before you add a type, schema, or helper" rule, use the existing factory: const ROOT_SOURCE = makeLogSource({ id: 'log-src', traceSourceId: 'trace-src', timestampValueExpression: 'Timestamp', resourceAttributesExpression: 'ResourceAttributes' }) here, and makeLogSource({ id: 'metric-source-1', name: 'Metrics', connection: 'conn-1', from: { databaseName: 'otel_v2', tableName: '' } }) at MetricTableModelForm.test.tsx:247 — which also removes the metricTables rest-destructure that only exists to dodge the excess-property check. That leaves shoehorn needed by at most DBTimeChart.test.tsx, so the dependency (and the fourth way to build a log-source fixture) can likely be dropped entirely.
There was a problem hiding this comment.
Addressed in 90b5b0e by importing makeLogSource directly from its existing module for both reviewed log fixtures and the nested source data in DBTimeChart. No factory move or new helper. The metric fixture now uses satisfies TMetricSource with all required fields. shoehorn remains only for partial query-hook results, as requested by #2852. The three affected suites passed 41 tests; lint, type checking, ratchet and commit hooks passed. Full-suite timeout details and successful isolated reruns are recorded in the PR description.
| }, | ||
| "app": { | ||
| "as-any": 215, | ||
| "as-any": 204, |
There was a problem hiding this comment.
🔵 minor — Baseline drop of 11 as any records no real reduction: fromPartial is an untracked escape hatch
fromPartial<TLogSource>({ id, kind, traceSourceId, timestampValueExpression, resourceAttributesExpression }) still hands back a value typed as a complete TLogSource while omitting the schema's required name, connection and from (packages/common-utils/src/types.ts:2187-2200) — exactly the unsoundness the removed as any had, just spelled with a helper scripts/ci/ratchet.mjs:15-25 does not count. That file's own docstring states the point of gating these counts is that "deleting an as any or an eslint-disable forces the underlying problem to surface"; here it does not, and the app baseline now reads 11 better with the same number of lying fixtures. Either add a 'from-partial': /fromPartial</g pattern to PATTERNS so the escape hatch stays visible, or build the fixtures with the complete factories above so the reduction is real.
There was a problem hiding this comment.
90b5b0e removes fromPartial from all source construction: logs use the existing complete factory and the metric fixture uses satisfies TMetricSource. Remaining fromPartial calls represent intentionally partial query-hook state/methods, not incomplete sources. The description now explicitly distinguishes syntactic ratchet counts from runtime validation. I have not added a repository-wide ratchet pattern for the helper that #2852 explicitly requests.
Deep Review✅ No critical issues found. This is a narrowly-scoped, test-only refactor: it swaps unsafe partial-mock assertions ( Independently verified as behavior-preserving and CI-safe:
🔵 P3 nitpicks (2)
Reviewers (6): correctness, testing, maintainability, project-standards, kieran-typescript, previous-comments. Testing gaps: none — no production behavior changed; all three affected suites are reported passing. |
| import { MantineProvider } from '@mantine/core'; | ||
| import { fireEvent, render, screen } from '@testing-library/react'; | ||
|
|
||
| import { makeLogSource } from '@/llm/__fixtures__/sources'; |
There was a problem hiding this comment.
🔵 minor — Shared source factory stays under the LLM feature folder and now collides with a same-named twin in the importing directory
After this PR makeLogSource has two definitions in play for components/__tests__/: the imported @/llm/__fixtures__/sources.ts:36 (used here, in DBTimeChart.test.tsx:20 and MetricTableModelForm.test.tsx:12) and a local one at packages/app/src/components/__tests__/ContextFilterPills.test.ts:13, which builds a different log source (timestampValueExpression: 'Timestamp', defaultTableSelectExpression: '*') via as unknown as TSource — the exact unsafe-assertion pattern this PR removes elsewhere. Move the factory to a package-level home (packages/app/src/__fixtures__/sources.ts), import it from the LLM tests too, and delete the ContextFilterPills twin, so one name means one fixture and generic component tests don't depend on an LLM-feature folder.
| jest.mocked(useSource).mockReturnValue({ | ||
| data: { id: 'test-source', name: 'Test Source' }, | ||
| } as any); | ||
| jest.mocked(useSource).mockReturnValue( |
There was a problem hiding this comment.
🔵 minor — Most mock returns in this file still bypass type checking via untyped as jest.Mock handles
The three fromPartial<ReturnType<typeof useSource>> sites are checked, but mockUseQueriedChartConfig/mockUseMe/mockUseSource (lines 57-59) are as jest.Mock, so the default mocks at lines 78 and 83 — and the later mockUseQueriedChartConfig.mockReturnValue calls at lines 355 and 555 — still accept any shape, which is the same escape hatch in a spelling the ratchet regex doesn't count. Drop the three cast handles and use jest.mocked(useQueriedChartConfig) / jest.mocked(useSource) with fromPartial for those returns too, so the whole file is covered by the new dependency you're paying for.
|
Merged the latest upstream The only conflict was Validation: focused suites 41/41 passed; The minor suggestions about moving the source factory and broad |
| "@testing-library/jest-dom": "^6.4.2", | ||
| "@testing-library/react": "^16.3.0", | ||
| "@testing-library/user-event": "^14.5.2", | ||
| "@total-typescript/shoehorn": "^0.1.2", |
There was a problem hiding this comment.
🔵 minor — New third-party devDependency added for a compile-time identity cast the repo already solves without one
fromPartial is only a typed identity cast, and it is used in exactly one file. The repo already has a zero-dependency idiom for this precise operation — mocking a hook whose real return is a full UseQueryResult from a stub that only carries data — documented at packages/app/src/components/DBEditTimeChartForm/__tests__/DBEditTimeChartForm.test.tsx:87-92: const mockUseSource = jest.requireMock<{ useSource: jest.Mock }>('@/source').useSource;. DBTimeChart.test.tsx itself already holds the equivalent handle (const mockUseSource = useSource as jest.Mock, line 59, used in beforeEach at line 78 with a partial object and no cast), so after this change the same hook is mocked two different ways in one file. Take the mockUseSource/requireMock handle for useSource and add a matching const mockUseMVOptimizationExplanation = ... handle, then drop @total-typescript/shoehorn from packages/app/package.json and the lockfile — it keeps the as any removal and the 204/142 ratchet baseline without adding supply-chain surface for a one-liner.
There was a problem hiding this comment.
Thanks for flagging the dependency tradeoff. #2852 explicitly requests fromPartial from @total-typescript/shoehorn so partial fixtures retain type checking for the fields they provide. Returning partial objects through an untyped jest.Mock handle would not preserve that check.
The current change uses the existing factories for complete source fixtures and keeps fromPartial only for partial query-hook results. I’m retaining this devDependency to match the issue’s requested approach. If maintainers prefer a dependency-free alternative, I’m happy to follow that direction.
2 participants
Summary
Latest validation (56c95d2)
Conflict resolution
Review follow-up
56c95d2 keeps makeLogSource in its existing module, replacing the two reviewed partial log fixtures and the nested log data in DBTimeChart. The metric source is checked for completeness with satisfies; type checking exposed missing fields, which are now explicit. No shared-factory refactor or new helper was needed.
The remaining fromPartial calls deliberately omit unused query-result state/methods, as requested by #2852. They are not source constructors or runtime validation. The global ratchet is unchanged.
The latest minor suggestions to move the source factory and to replace every remaining jest.Mock assertion are adjacent cleanup outside this issue-scoped change, so they are intentionally deferred and do not block this PR.
Preview
N/A: non-UI test maintenance. Vercel authorization and vouch remain maintainer-side processes.
Fixes #2852
Related: #2890 (closed)
Implemented and reviewed with Codex assistance.