Skip to content

review: restore sparkline peak fallback, make api_change test self-seeding - #473

Merged
tonyalaribe merged 1 commit into
masterfrom
deep-clean-followup
Jul 30, 2026
Merged

review: restore sparkline peak fallback, make api_change test self-seeding#473
tonyalaribe merged 1 commit into
masterfrom
deep-clean-followup

Conversation

@tonyalaribe

Copy link
Copy Markdown
Contributor

Post-merge follow-up to #472, addressing the final review round:

  • Components: sparkline_ peak index falls back to 0 when no bucket matches peakVal (all-negative series, where the foldr max 1 seed wins) instead of length buckets, which pushed the marker off-chart.
  • AnomaliesSpec: the defaultRecommendedAction regression test now seeds an api_change issue through the real pipeline when run in isolation, instead of depending on earlier tests in the file.

The other two findings from that round were analyzed and answered on #472: the dropped Stripe rows > 0 guard is provably equivalent to the whenJustM projectBySubId it merged into (the UPDATE has no plan predicate), and the defaultRecommendedAction wording change is intentional now that the string is shared across issue types.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ExECCj9BwAnXC7GkVSKtwx

…eding

- Components: peakIdx falls back to 0 when no bucket matches peakVal
  (all-negative series, where the foldr seed 1 wins) instead of
  length buckets, which pushed the marker off-chart
- AnomaliesSpec: defaultRecommendedAction test seeds via the real
  pipeline when run in isolation instead of relying on earlier tests
@tonyalaribe
tonyalaribe merged commit 0e5df87 into master Jul 30, 2026
9 checks passed
@tonyalaribe
tonyalaribe deleted the deep-clean-followup branch July 30, 2026 23:11
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review

Small, well-scoped follow-up to #472 addressing the last two review findings. Both changes look correct.

sparkline_ peak-index fix (src/Pages/Components.hs)

  • peakIdx = maybe 0 fst $ find ((== peakVal) . snd) $ zip [0 ..] buckets correctly fixes the fallback: the old length $ takeWhile (/= peakVal) buckets degenerates to length buckets (an out-of-bounds index) when nothing matches, rather than 0. Good catch, and the inline comment explains the non-obvious "seed 1 wins" scenario well.
  • find/zip are already used idiomatically elsewhere in the codebase via Relude, so no new imports needed.
  • Minor/optional: since peak = fromIntegral peakVal is computed right above for bar-height scaling, the index could be found in one pass with elemIndex peakVal buckets instead of find ((== peakVal) . snd) . zip [0..] — reads a bit more directly as "find peakVal's position" and drops the tuple destructuring:
    peakIdx = fromMaybe 0 $ elemIndex peakVal buckets
    Purely stylistic — both are O(n) and correct; elemIndex is just slightly more idiomatic for this.

AnomaliesSpec self-seeding test (test/integration/Pages/AnomaliesSpec.hs)

  • Removing the inter-test ordering dependency is the right call for isolation/robustness (e.g. running with --match or reordering it blocks).
  • The seeding logic (getCurrentTimetestRequestMsgs.reqMsg1processMessagesAndBackgroundJobsrunBackgroundJobsWhere ... NewAnomaly{}) mirrors the exact pattern used earlier in the same file (e.g. lines ~70-93, ~147-166), and the Only Issues.IssueId row-type annotation matches existing precedent throughout the file — consistent with established conventions.
  • The when (null existing) guard avoids double-seeding when the earlier tests in the file do run first, so no risk of duplicate/conflicting rows.

Other notes

  • Diff is minimal (16 additions / 1 deletion) and tightly scoped to the two flagged issues; no unrelated changes.
  • No security or performance concerns — this is UI marker math and test-only code.
  • The PR description's rationale for not changing the other two flagged items (Stripe rows > 0 guard, defaultRecommendedAction wording) is sound and appropriately deferred to the refactor: deep-clean sweep across Pages, Models, and Components #472 discussion rather than re-litigated here.

Nothing blocking. Nice, targeted cleanup.

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