Skip to content

fix(inference): stop flagging a complete comparison date range / 修复对比日期范围的误报 - #702

Merged
edwingao28 merged 4 commits into
masterfrom
edwingao28/fix-comparison-range-false-alarm
Aug 10, 2026
Merged

fix(inference): stop flagging a complete comparison date range / 修复对比日期范围的误报#702
edwingao28 merged 4 commits into
masterfrom
edwingao28/fix-comparison-range-false-alarm

Conversation

@edwingao28

@edwingao28 edwingao28 commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

Following an Overview 30-day cell's Compare curves link landed on /inference with the Comparison Date Range picker flashing red, even though the comparison was fully specified and both curves were already on the chart.

Repro: https://inferencex.semianalysis.com/overview?compare=30d → any history cell → Compare curves.

Root cause

The error state was gated on the date range alone:

selectedGPUs.length > 0 && (!selectedDateRange.startDate || !selectedDateRange.endDate)

But the range is only one of two additive sources of comparison series — resolveComparisonEntries(selectedDates, range) also takes explicit entries from i_dates, and a pinned run (2026-06-10~r27489075807) can only be expressed that way, never as a range. buildOverviewHistoryDashboardHref (#686) correctly pins both runs through i_dates and sets no i_dstart/i_dend, so the chart was complete while the picker read empty and raised a false alarm.

Fix

Gate the error state on the same predicate the chart itself uses:

resolveComparisonEntries(selectedDates, selectedDateRange).length === 0

This covers every entry point that supplies dates without a range, not just the Overview link. The nudge still fires when a chip config is selected with nothing to compare against.

Testing

  • New component tests: flagged when nothing is picked; unflagged when exact entries are pinned.
  • New e2e test walking the real flow (/overview?compare=30dCompare curves → assert the picker is not flagged). Verified it fails on master with exactly the reported symptom, so it is not vacuous.
  • typecheck / lint / fmt clean; 480 unit, 198 component, overview.cy.ts + inference-chart.cy.ts green.
  • Browser-verified both directions against a fixture-backed dev server.

中文说明

从总览页 30 天单元格的 Compare curves 链接跳转到 /inference 后,即使对比配置已经完整、两条曲线也已经画在图上,Comparison Date Range 选择器仍然会闪红。

复现路径:https://inferencex.semianalysis.com/overview?compare=30d → 任意历史单元格 → Compare curves

根因:错误状态此前只根据日期范围判断。但日期范围只是对比曲线的两个叠加来源之一 —— resolveComparisonEntries(selectedDates, range) 同样接受来自 i_dates 的显式条目,而固定运行(2026-06-10~r27489075807)只能通过这种方式表达,无法用范围表示。buildOverviewHistoryDashboardHref#686)正确地用 i_dates 固定了两次运行且不设置 i_dstart/i_dend,于是图表是完整的,选择器却显示为空并误报。

修复:改用图表自身使用的判定条件 resolveComparisonEntries(selectedDates, selectedDateRange).length === 0。这覆盖了所有"只给日期、不给范围"的入口,而不仅是总览页链接;当选中 chip 配置但确实没有任何对比对象时,红色提示仍会正常出现。

测试:新增 2 个组件测试与 1 个走完整流程的 e2e 测试;已确认该 e2e 测试在 master 上会以完全相同的症状失败。typecheck / lint / fmt 通过,480 个单元测试、198 个组件测试、overview.cy.tsinference-chart.cy.ts 全绿,并在基于 fixture 的本地服务上做了双向浏览器验证。


Note

Low Risk
UI validation logic only; behavior is narrowed to match existing chart resolution with added regression tests.

Overview
Fixes a false alarm on the inference Comparison Date Range control: the red pulse no longer appears when GPUs are selected but the range is empty while selectedDates / i_dates already define a complete comparison (e.g. Overview Compare curves links that pin runs like date~rID).

ChartControls now gates the nudge with resolveComparisonEntries(selectedDates, selectedDateRange).length === 0, the same predicate the chart uses, instead of requiring startDate/endDate. The warning still shows when chip configs are selected with nothing to compare against.

Tests: Cypress component cases for flagged vs unflagged picker state, plus an e2e path from /overview?compare=30d through a history Compare curves link.

Reviewed by Cursor Bugbot for commit 5fd1dc2. Bugbot is set up for automated code reviews on this repo. Configure here.

…日期范围的误报

Overview's per-cell "Compare curves" link pins both runs through i_dates and
sets no i_dstart/i_dend, so the picker read empty and rendered the red pulsing
error state even though the comparison was fully specified. The range is only
one of two additive sources of comparison series, and a pinned run can only be
expressed through i_dates — so gate the error state on resolveComparisonEntries,
the same predicate the chart itself uses.

中文:总览页每个单元格的“Compare curves”链接通过 i_dates 固定两次运行,不设置
i_dstart/i_dend,因此日期选择器显示为空并触发红色闪烁的错误状态,尽管对比其实已经
完整。日期范围只是对比曲线的两个叠加来源之一,而固定运行只能通过 i_dates 表达,
因此改用图表自身使用的 resolveComparisonEntries 作为判定条件。
@edwingao28
edwingao28 requested a review from adibarra as a code owner August 9, 2026 05:29
@vercel

vercel Bot commented Aug 9, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
inferencemax-app Ready Ready Preview Aug 10, 2026 9:38pm

Request Review

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @edwingao28's task in 1m 37s —— View job


🟢 LGTM — no blocking issues found

Reviewed the fix, tests, CI, and verified the flow at runtime.

The fix is correct and well-scoped. The error state now gates on resolveComparisonEntries(selectedDates, selectedDateRange).length === 0 — the exact predicate the chart itself uses in useChartData.ts:73 to build the comparison series. This is the right coupling: the picker turns red iff the chart genuinely has nothing to compare, regardless of whether the entries came from a date range or from pinned i_dates runs.

Edge cases hold up:

  • Dropping the old selectedGPUs.length > 0 guard is safe — the whole block is already inside {!hideGpuComparison && selectedGPUs.length > 0 && (…)} at ChartControls.tsx:604, so it's redundant.
  • A range whose endpoints are both shadowed by same-day run entries still yields a non-empty result (the runs remain in selectedDates), so it correctly stays unflagged.
  • A valid range with empty selectedDates → length ≥ 1 → unflagged. Correct.

Runtime verification (real-data dev server): loaded /overview?compare=30d → grabbed a real history-cell link (i_dates=2026-07-12,2026-07-01~r28403258484, no i_dstart/i_dend) → navigated to it. Result: Comparison Date Range label visible, no animate-pulse on the picker, 0 console errors. Matches the intended fix.

Tests: regression coverage in both directions — component tests for the flagged (nothing picked) and unflagged (pinned entries) states, plus an e2e that walks the real Overview → Compare-curves flow. Good.

CI: Lint & Format ✅, Tests (Unit) ✅; Tests (E2E) still in progress at review time.

  • Gather context
  • Analyze the fix for correctness & edge cases
  • Verify test coverage
  • Check CI status
  • Runtime verify the flow
  • Post review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 070d31c5e7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/app/src/components/inference/ui/ChartControls.tsx
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @edwingao28's task in 3m 33s —— View job


Re-review — 🟢 LGTM (one non-blocking nit)

The core fix is correct and I verified the reported flow at runtime. The picker now gates on resolveComparisonEntries(selectedDates, selectedDateRange).length === 0, which matches how the chart's comparison series is assembled far more closely than the old range-only check — so a comparison completed via pinned i_dates runs (no i_dstart/i_dend) no longer raises a false alarm.

Runtime verification (real-data dev server): /overview?compare=30d → real history-cell link (g_rundate=2026-07-12, i_dates=2026-07-12,2026-07-01~r28403258484, no range) → navigated. The Select date range button is not pulsing (no animate-pulse, no ring-red), 0 console errors. ✅

CI: Lint & Format ✅, Tests (Unit) ✅, Tests (E2E) ✅ (the merge commit's runs are all green; only my own Claude job is still in progress).

One non-blocking finding (independently confirms Codex's P2 on ChartControls.tsx:621): the picker uses resolveComparisonEntries, but the chart draws from buildComparisonDates(...), which also excludes the main run (selectedRunDate / comparisonMainRunId). When selectedDates holds only the main run, the picker won't pulse even though no comparison curve renders. I checked the real Overview links — they always pin a distinct second entry, so this edge state isn't reachable via the reported flow; it's a precision nit, not a regression. Suggested exact-predicate fix is in the inline comment.

Tests: regression coverage in both directions (flagged when nothing picked; unflagged when exact entries pinned) plus the Overview→Compare-curves e2e. Good.

  • Gather context (diff, comparisonEntry, useChartData)
  • Assess Codex P2 finding (main-run filtering) — confirmed, non-blocking
  • Runtime-verify the flow — no pulse, 0 console errors
  • Check CI — all green
  • Post review

Comment thread packages/app/src/components/inference/ui/ChartControls.tsx
@edwingao28
edwingao28 enabled auto-merge (squash) August 10, 2026 07:27
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @edwingao28's task in 55s —— View job


Re-review — 🟢 LGTM (no new issues)

The push that triggered this re-review only added merge commits from master (e247f9d, d396316) — the fix itself (ChartControls.tsx, 070d31c) is byte-identical to what the two prior reviews already covered and verified at runtime. Nothing new to flag.

Fix still correct. The picker gates on resolveComparisonEntries(selectedDates, selectedDateRange).length === 0 (ChartControls.tsx:621) — the same additive source (i_dates entries + range endpoints) the chart uses — so a comparison pinned purely through i_dates runs (no i_dstart/i_dend) no longer false-alarms. Dropping the old selectedGPUs.length > 0 guard remains safe: the block is already inside !hideGpuComparison && selectedGPUs.length > 0.

Previously-raised WARNING still open (unchanged, non-blocking). Both Codex (P2) and the earlier Claude review noted the picker uses resolveComparisonEntries while the chart draws from buildComparisonDates (useChartData.ts:73), which also drops entries equal to selectedRunDate / selectedRunId. So if selectedDates holds only the main run, the picker stays quiet while no comparison curve renders. This edge state isn't reachable from the real Overview links (they always pin a distinct second entry), so it's a precision nit, not a regression. Not re-anchoring an inline comment since it already exists on ChartControls.tsx:621.

CI: Lint & Format ✅, Tests (Unit) ✅, Code Quality ✅. Tests (E2E) still in progress at review time.

Tests: regression coverage in both directions (flagged when nothing picked; unflagged when exact entries pinned) plus the /overview?compare=30dCompare curves e2e. Good.

  • Gather context (diff, new commits since last review)
  • Assess the resolveComparisonEntries vs buildComparisonDates finding — still open, non-blocking
  • Check CI status
  • Post review

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @edwingao28's task in 1m 4s —— View job


Re-review — 🟢 LGTM (no new issues)

The push that triggered this re-review added only a master merge (5fd1dc2) pulling in an unrelated change (3051dcb, the agentic percentile-selector feature gate). The fix itself in ChartControls.tsx is byte-identical to what the three prior reviews already covered and verified at runtime — git diff e247f9d..HEAD on that file shows only the PercentileSelector gate edit, not the comparison-range block. Nothing new to flag.

Fix still correct. The picker gates the red pulsing border on resolveComparisonEntries(selectedDates, selectedDateRange).length === 0 (ChartControls.tsx:624) — the same additive source (i_dates entries + range endpoints) the chart consumes via buildComparisonDates (useChartData.ts:73). So a comparison pinned purely through i_dates runs with no i_dstart/i_dend (the Overview Compare curves flow) no longer false-alarms. Dropping the old selectedGPUs.length > 0 guard remains safe — the block is already nested inside !hideGpuComparison && selectedGPUs.length > 0.

Previously-raised WARNING still open (unchanged, non-blocking). Both Codex (P2) and the earlier Claude reviews noted the picker uses resolveComparisonEntries while the chart's series come from buildComparisonDates, which additionally drops entries equal to selectedRunDate / selectedRunId (useChartData.ts:74-76). If selectedDates ever holds only the main run, the picker stays quiet while no comparison curve renders. Not reachable from the real Overview links (they always pin a distinct second entry), so it's a precision nit, not a regression. Not re-anchoring an inline comment — the existing one on ChartControls.tsx:624 still stands.

CI: Lint & Format ✅, CodeQL ✅, Cursor Bugbot ✅, CodeQL/Analyze ✅. Typecheck+unit and E2E shards still pending at review time — the dedicated workflows gate the merge.

Tests: unchanged, regression coverage in both directions (component: flagged when nothing picked, unflagged when exact entries pinned) plus the /overview?compare=30dCompare curves e2e that was confirmed to fail on master. Good.

  • Gather context (diff, new commits since last review)
  • Re-assess open resolveComparisonEntries vs buildComparisonDates finding — still open, non-blocking
  • Check CI status
  • Confirm fix unchanged since last verified review (no re-run needed)
  • Post review

@edwingao28
edwingao28 merged commit 054a9d3 into master Aug 10, 2026
24 checks passed
@edwingao28
edwingao28 deleted the edwingao28/fix-comparison-range-false-alarm branch August 10, 2026 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant