Fix chart hover band alignment - #3100
Conversation
Align CostHistory and breakdown hover frames with BarMark geometry by using bucketCalendar for day intervals and pixel-snapping bars to the backingScaleFactor. Fixes visible shadow/column offset in token and cost history charts.
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76be4ffb39
ℹ️ 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".
| let centerX = plotFrame.minX + (interval.lowerBound + interval.upperBound) / 2 | ||
| let rawWidth = (interval.upperBound - interval.lowerBound) * widthRatio | ||
| let rawCenterX = plotFrame.minX + (interval.lowerBound + interval.upperBound) / 2 | ||
| let width = (rawWidth * scale).rounded() / scale |
There was a problem hiding this comment.
At a 2× backing scale (including the headless fallback), the existing calendar day bars use interval midpoint instead of interval start test supplies a 23-point interval, so this rounding changes the expected width from 16.1 to 16.0; its 0.0001-tolerance assertion therefore fails. Update that focused test to expect the snapped geometry, or make the scale injectable and assert each supported scale.
AGENTS.md reference: AGENTS.md:L23-L24
Useful? React with 👍 / 👎.
Evidence (pre-fill for ClawSweeper)Root cause: Fix:
No data/model change, only overlay geometry. Local verification:
Visual proof:
Risk: low, overlay-only; |
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs real behavior proof before merge. Reviewed August 20, 2026, 9:20 AM ET / 13:20 UTC. ClawSweeper reviewWhat this changesThis PR aligns usage-chart hover bands with bar geometry by using display-day calendars and snapping overlay frames to pixel boundaries. Merge readiness⛔ Blocked until real behavior proof is added - 10 items remain Keep open: the updated head still has two deterministic test failures, uses the wrong screen scale for a chart on a secondary display, and lacks inspectable after-fix UI proof. Priority: P3 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherCodexBar renders provider usage as Swift Charts in its macOS menu. Usage dates and chart layout produce hover-band frames that both highlight and select daily bars. flowchart LR
A[Provider usage dates] --> B[Swift Chart bars]
B --> C[Display-day intervals]
C --> D[Hover frame geometry]
D --> E[Pixel snapping]
E --> F[Hover highlight and selection]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Rebase onto current main, restore exact gatekeeper anchors, pass the rendering chart’s display scale into snapping with 1× and 2× coverage, then attach redacted after-fix visual proof. Do we have a high-confidence way to reproduce the issue? Yes: source inspection gives high-confidence deterministic reproduction for the 1× assertion and the exact-line gatekeeper failures; the secondary-display path is also directly identifiable from the main-screen scale lookup. Is this the best way to solve the issue? No: the calendar correction is appropriately narrow, but snapping must use the display hosting the chart and the accidental gatekeeper edits must be corrected before this is a maintainable fix. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 2cb9efda1d1a. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
- CostHistory hover now uses displayCalendar (gregorian current) not bucketCalendar, so Mistral UTC keys already mapped to local display dates do not shift by timezone offset - ChartBarHoverSelection.bars gains scale param (default 2x) for deterministic tests; DST case now expects snapped 16.0 at 2x and verifies 16.1 at 1x
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d87ce7b1ff
ℹ️ 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".
| line: 392, | ||
| anchor: "provider: .codex,", |
There was a problem hiding this comment.
In this commit's SpendDashboardController.swift, the referenced provider: .codex, remains on line 385, while this suppression is changed to line 392, which contains inputs: openCodex.inputs; the gatekeeper explicitly requires the anchor to match the configured source line, so this entry—and the similarly shifted entries below it—produces deterministic failures in cross provider case clusters are derived or specifically justified. Restore these numbers to the lines in the reviewed source tree rather than those from a different revision.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
| widthRatio: 0.7, | ||
| scale: 1) | ||
| let bar = try #require(bars.first) | ||
| #expect(abs(bar.frame.width - (23 * ChartBarHoverSelection.barWidthRatio)) < 0.0001) |
There was a problem hiding this comment.
With the newly added scale: 1 input, bars computes (23 * 0.7).rounded() / 1, which is 16, but this assertion compares it with the unsnapped value 16.1, so the focused test always fails. This explicit 1× case is fresh evidence beyond the earlier 2× assertion; update its expectation to the snapped result.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
| unitIntervals: [ClosedRange<CGFloat>], | ||
| widthRatio: CGFloat = ChartBarHoverSelection.barWidthRatio) -> [Bar] | ||
| widthRatio: CGFloat = ChartBarHoverSelection.barWidthRatio, | ||
| scale: CGFloat = NSScreen.main?.backingScaleFactor ?? 2) -> [Bar] |
There was a problem hiding this comment.
When the menu is shown on a monitor whose backing scale differs from the main monitor—for example, a 1× external display paired with a 2× main display—NSScreen.main supplies the wrong quantization scale for the chart being rendered. The resulting hover/highlight frames are rounded to different point boundaries than the BarMark, recreating the alignment and hit-testing drift this change is intended to remove; pass the view's actual display scale into this helper instead.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
1 participant
Align CostHistory and breakdown hover frames with BarMark geometry.
Fixes the shadow/column offset seen in 8-20 screenshots (606M token, $0.22 cost). No data change, only overlay geometry.
Test: swift build passes; visual check of 18-bar token chart and single-day cost cap centered.