Skip to content

Fix spend chart date inspection and source visibility - #4329

Merged
steipete merged 4 commits into
steipete:mainfrom
Yuxin-Qiao:fix/spend-chart-scope
Oct 8, 2026
Merged

steipete merged 4 commits into
steipete:mainfrom
Yuxin-Qiao:fix/spend-chart-scope

Conversation

@Yuxin-Qiao

@Yuxin-Qiao Yuxin-Qiao commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Spend-chart pointer inspection could round plot padding to an adjacent date with no bucket. Resolve daily inspection to existing buckets inside the plotted domain, restrict hourly navigation to recorded days in the reporting range, and recover stale date/source selections.

Scope the wrapping legend to sources with recorded amounts in the displayed overview, drilled interval, or hourly day. Recorded zero-dollar sources remain visible and selectable, and zero-dollar inspector rows remain visible. Account labels/colors continue to use the full source list. Stack construction reuses source identity records without changing accounting totals.

Maintainer changes preserve the contributor commits and add DST inspection and zero-only source coverage. Thanks @Yuxin-Qiao!

Verification

  • make check: passed; zero format/lint violations.
  • source Scripts/test_environment.sh followed by swift test --build-system native --jobs 4 -Xswiftc -gnone --filter 'SpendTrendScopeTests|SpendTrendChartTests|SpendTrendPresentationRegressionTests|SpendTrendCalendarTests|SpendTrendOverflowTests|SpendStackedBarChartTests': 35 tests passed on the final head.
  • CODEXBAR_SPEND_TREND_PROOF_DIR=... with SpendTrendChartRenderTests: one native render test passed, producing 14 synthetic captures.
  • Baseline c3c6ce06120: a behavior-preserving extraction of the existing overlay date calculation plus scope tests failed 21 assertions. The contributor's positive-only filter separately failed three zero-source assertions; all pass after the fix.
  • Independent review: no actionable P0–P2 findings. Production diff versus the reviewed main baseline: +60/−60 lines.

Final combined source proof at 8f564053f16d0b1a027e35828dca44c49df34d2c:

source Scripts/test_environment.sh
CODEXBAR_TEST_SUITE_TIMEOUT=600 ./Scripts/test.sh \
  --swift-command "$PWD/../reports/spend-prs-23-evidence/swift-native" --direct-workers 4

The wrapper forwards test/build to swift with --build-system native --jobs 4 -Xswiftc -gnone. 1,592 selections, 144/144 groups successful on the first attempt, zero retries and zero timeouts. The earlier 180-second invocation stopped on an unrelated publication-suite timeout; source and assertions were unchanged for the successful rerun. The pushed integration branch is triage/20260921-spend-prs-23; its later commit adds only the contributor's runtime-proof documents.

Synthetic before/after

Before the zero-dollar correction:

Recorded zero-dollar Claude source hidden by the original positive-only filter

After:

Recorded zero-dollar Claude source and amount retained with unchanged total

These are isolated production-component renders, not a packaged-app pointer recording. Fixture details and reproduction commands: docs/screenshots/spend-chart-scope/README.md.

Refs #4298.

Current head: 9e0ee8091e87dce25848925108dcdfdd632ea646. CI for this head was queued at the final status check.

@clawsweeper

clawsweeper Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Oct 7, 2026
@clawsweeper

clawsweeper Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed October 7, 2026, 9:54 PM ET / October 8, 2026, 01:54 UTC (Revision 3).

ClawSweeper review

What this changes

The branch constrains spend-chart date inspection, scopes and wraps source legends, and recovers stale selections while retaining recorded zero-dollar amounts.

Example: Hover October 4, 2026 in the sparse daily chart.

  • Before: The inspector can select October 4 even though that date has no recorded bucket.
  • After: October 4 is rejected as an inspection selection.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A useful, focused patch with meaningful regressions remains limited by incomplete proof of the changed native interactions.
Proof confidence 🦐 gold shrimp (3/6) Needs stronger real behavior proof before merge: The inspected current screenshots demonstrate corrected zero-dollar rendering in SpendDashboardTrendPanel’s isolated native component harness. They do not exercise the changed ChartProxy hover/tap path or interactive scope recovery in a running app; the full-Settings captures predate this fix. No stored-data contract changes require migration proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Hovering chart padding can show an empty adjacent date, and stale date or account selections can leave users inspecting an unavailable scope.
Reason: The selection repair addresses a concrete failure in existing chart exploration. Maintainer follow-up explicitly supports the accompanying scoped legends and recorded-zero preservation.

Merge readiness

⛔ Blocked before merge - 2 items remain

This PR needs real behavior proof before merge. It remains useful on current main, and no actionable correctness defect was found; the earlier interactive-proof blocker remains unresolved.

Priority: P2
Reviewed head: 9e0ee8091e87dce25848925108dcdfdd632ea646

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The inspected current screenshots demonstrate corrected zero-dollar rendering in SpendDashboardTrendPanel’s isolated native component harness. They do not exercise the changed ChartProxy hover/tap path or interactive scope recovery in a running app; the full-Settings captures predate this fix. No stored-data contract changes require migration proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Complete next step (P2) - Provide after-fix screenshots, a recording, or a runtime transcript from a freshly built running app demonstrating padding hover/tap rejection and date/source scope recovery. Redact private account information, IP addresses, API keys, and non-public endpoints. Updating the PR body triggers a fresh review; otherwise a maintainer can comment @clawsweeper re-review.

Findings

None.

Tests

  • Missing end-to-end proof: After-fix evidence from a freshly built running app must show padding hover/tap rejection and date/source scope recovery. Component screenshots show corrected zero-dollar rendering, but do not exercise those interactions. The reported base-failing/head-passing model checks were not executed in this read-only review.
Agent review details

How this fits together

CodexBar’s Usage & Spend settings turn recorded account history into spending charts and exact-amount inspectors. This change governs which dates and sources users can inspect within the displayed reporting period.

flowchart TD
  A[Recorded account amounts] --> C[Spend chart presentation]
  B[Reporting period and time zone] --> C
  C --> D[Available date buckets and sources]
  E[Pointer and source selections] --> F[Validate selection]
  D --> F
  F --> G[Chart and exact amount inspector]
Loading

Technical review

Best possible solution:

Keep selection validation and source scoping in the existing chart presentation layer, preserving recorded-zero, missing-cost, and stable account-identity semantics.

Do we have a high-confidence way to reproduce the issue?

Yes, source establishes the path: the base rounds pointer dates without checking the plotted domain or recorded daily buckets. The scope fixtures provide concrete boundary cases; this read-only review did not execute them.

Is this the best way to solve the issue?

Yes, the patch reuses the existing chart model and wrapping layout, preserves accounting semantics, and has explicit maintainer direction for scoped source visibility.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 844b0e19bbbb.

Provenance checked

  • Sources/CodexBar/SpendDashboardTrendPanel.swift: pointer inspection changes intended behavior with a stated reason (6bdaac6: The original chart feature added hover inspection, persistent exact amounts, and date drill-down.)
  • Sources/CodexBar/SpendDashboardTrendPanel.swift: legend membership and wrapping changes intended behavior with a stated reason (Improve spend chart readability and drill-down #4298: The merged feature made same-provider accounts distinguishable and selectable while retaining stable account colors.)
  • Sources/CodexBar/SpendTrendChartModel.swift: hourly dates and focused-day recovery changes intended behavior with a stated reason (6bdaac6: The feature introduced single-day hourly exploration and navigation through recorded days.)
  • Sources/CodexBar/SpendTrendChartModel.swift: source aggregation keeps the original intent (Improve spend chart readability and drill-down #4298: Recorded chart sums must preserve source identity, unavailable accounting totals, and finite aggregation.)

Testing

Proof path: in-process harness. Added test files: 3.

Security

None.

Evidence

What I checked:

  • Applicable repository policy: Read the complete root AGENTS.md. The repository contains no additional AGENTS.md files or matching maintainer-note files. Applied its guidance on small changes, existing layout helpers, provider identity separation, and fresh-build UI validation; no builds or tests were executed during this read-only review. (AGENTS.md:1, 9e0ee8091e87)
  • Verified introduced changes: Inspected all eight introduced files using the pinned merge-base-to-head delta. Production changes are limited to chart presentation; accounting totals, persisted settings, provider fetching, dependencies, and build automation are unchanged. (Sources/CodexBar/SpendTrendChartModel.swift:48, 9e0ee8091e87)
  • Still necessary on current main: The two production files are unchanged between the pinned base and fetched main. Current main still lacks domain-and-bucket inspection validation, scoped legend membership, and the in-range hourly-day filtering introduced here. The merged Improve spend chart readability and drill-down #4298 established the chart feature rather than fixing these follow-up problems. (Sources/CodexBar/SpendTrendChartModel.swift:251, 844b0e19bbbb)
  • Feature history and owner direction: History searches connect the chart feature to 6bdaac6 and subsequent accounting/calendar contributions. Read the original merged PR’s stated goals and owner comment. The current maintainer-authored follow-up explicitly preserves zero-dollar sources, scoped dates, and contributor history, covering the proposed presentation direction. (Sources/CodexBar/SpendDashboardTrendPanel.swift:6, 6bdaac65abbc)
  • History fallback: Local git show of the feature commit failed because a missing promisor object returned HTTP 403. The read-only GitHub commit API supplied the original added-file patches; raw commit records and local log/shortlog remained readable. No introduction attribution is inferred from blame alone. (Sources/CodexBar/SpendTrendChartModel.swift, 6bdaac65abbc)
  • Inspected corrected screenshot proof: Inspected both prepared attachment images. The corrected hourly component render visibly retains Claude $0.00 beside Codex $4.00 and $5.00, with the same recorded total of at least $9.00. The documented NSHostingView render does not exercise pointer input or scope transitions in the shipped Settings window. (docs/screenshots/spend-chart-scope/README.md:33, 9e0ee8091e87)

Likely related people:

  • Yuxin-Qiao: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Review metrics

Metric Value Why it matters
Production and test changes Production Swift +60/−60 lines; tests +173/−0 lines The bounded presentation repair has no net production growth and adds focused scope and rendering coverage.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: This is a bounded spend-chart inspection and readability repair without an urgent availability or onboarding failure.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The inspected current screenshots demonstrate corrected zero-dollar rendering in SpendDashboardTrendPanel’s isolated native component harness. They do not exercise the changed ChartProxy hover/tap path or interactive scope recovery in a running app; the full-Settings captures predate this fix. No stored-data contract changes require migration proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence.

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add redacted running-app screenshots or a recording showing padding inspection, date/source scope changes, and retained zero-dollar entries.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only; repair and merge need explicit maintainer commands such as @clawsweeper autofix or @clawsweeper automerge.

History

Review history (2 earlier review cycles)
  • reviewed 2026-10-07T15:35:00.342Z sha 1e9c06b :: needs real behavior proof before merge. :: none
  • reviewed 2026-10-08T00:17:08.627Z sha 9e0ee80 :: needs real behavior proof before merge. :: none

Keep date inspection within plotted buckets, retain recorded zero-dollar source chips and inspector rows, and cover hourly inspection across DST. Reuse source records during stacking and sync the reviewed main baseline without rewriting contributor history.

Co-authored-by: Yuxin Qiao <104957188+Yuxin-Qiao@users.noreply.github.com>
@steipete steipete changed the title Fix spend chart date inspection and show only active sources Fix spend chart date inspection and source visibility Oct 8, 2026
@steipete
steipete merged commit 5665272 into steipete:main Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants