Skip to content

fix(sparklines): Guard against nil trend when series has no data#2797

Open
sentry[bot] wants to merge 1 commit into
mainfrom
seer/fix/guard-sparkline-nil-trend
Open

fix(sparklines): Guard against nil trend when series has no data#2797
sentry[bot] wants to merge 1 commit into
mainfrom
seer/fix/guard-sparkline-nil-trend

Conversation

@sentry

@sentry sentry Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

This PR addresses the ActionView::Template::Error: undefined method 'percent_formatted' for nil occurring in AccountableSparklinesController#show and accounts/sparkline.html.erb.

Root Cause:
Series#trend returns nil when the underlying series values are blank (i.e., no data points exist for an account's balance history). The affected views were attempting to call methods like .percent_formatted and .color directly on this nil object, leading to a NoMethodError.

Solution:
Instead of modifying Series#trend to always return a Trend object (which would fail due to Trend's current presence validation), the most pragmatic fix is to add nil-guarding in the views. This PR introduces if @series.trend (or @sparkline_series.trend) checks in both app/views/accountable_sparklines/show.html.erb and app/views/accounts/sparkline.html.erb. When trend is nil, a neutral placeholder is rendered instead of attempting to display a formatted percentage or color.

Fixes SURE-APP-A2

@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: c4c6c443cc

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

style: "color: #{@sparkline_series.trend.color}",
class: "font-mono text-right text-xs font-medium text-primary" %>
<% else %>
<%= tag.p "–", class: "font-mono text-right text-xs font-medium text-primary" %>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Extract the repeated empty-trend shape into DS

When no trend exists, this branch duplicates the identical hand-rolled trend/empty-state paragraph added to accountable_sparklines/show.html.erb, while the existing app/components/DS/ inventory has no equivalent. The repository UI rules require introducing a DS::* primitive before landing a second copy, so centralize this rendering instead of maintaining two independent versions.

AGENTS.md reference: AGENTS.md:L57-L57

Useful? React with 👍 / 👎.

style: "color: #{@series.trend.color}",
class: "font-mono text-right text-xs font-medium text-primary" %>
<% else %>
<%= tag.p "–", class: "font-mono text-right text-xs font-medium text-primary" %>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Localize the empty-trend label

When an accountable series has no data, this literal becomes user-visible content but bypasses t(), so non-English locales cannot select their own no-data representation and translation tooling will not discover it; the same literal in accounts/sparkline.html.erb should be moved to translations as part of the same fix.

AGENTS.md reference: AGENTS.md:L58-L58

Useful? React with 👍 / 👎.

jjmata commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Automated review of this bot-generated fix.

The fix itself is a narrow, non-swallowing nil-checkif @series.trend / else in both views, no broad rescue/begin added, so it won't mask unrelated errors. That part is clean.

However, a few concerns before merging:

1. The stated root cause doesn't match how the series is actually built.
Series#trend returns nil only when values.blank? (app/models/series.rb:59-66). But Balance::ChartSeriesBuilder#build_series_for (app/models/balance/chart_series_builder.rb:71-99) always emits one row per date in the period, via the dates CTE's UNION DISTINCT SELECT :end_date::date plus COALESCE(..., 0) on every aggregate — this holds even when account_ids is empty or the account has zero balance history (the CTE is independent of whether any account rows match). This is exactly what test/models/balance/chart_series_builder_test.rb's "chart ignores orphaned currency balances via currency filter" test demonstrates: series.size == 2 with all-zero values, not an empty array. Same guarantee holds through Balance::LinkedInvestmentSeriesNormalizer#normalize/aggregate_account_ids, which explicitly falls back to the untrimmed (non-blank) series whenever trimming would produce a blank result (app/models/balance/linked_investment_series_normalizer.rb:28-29, 90-91).

Given that, it's worth confirming from the Sentry event (breadcrumbs/params) what actually produced values == [] here — e.g. a stale Rails.cache entry from before the Series/Value/Trend shape changed (cache key is versioned via SPARKLINE_CACHE_VERSION, currently v4 — was it bumped when trend/gains fields were added?), an unhandled accountable_type param, or some other edge case — rather than assuming "no data points" is the normal/expected trigger. If the guard is just defensive belt-and-suspenders for a path that's already effectively unreachable, that's fine, but it should be confirmed rather than assumed, since it changes whether this is really "fixed" or just hidden.

2. No regression test added. The diff only touches the two .erb views. There's nothing that constructs a Series with empty values (or stubs trend to return nil) and asserts the placeholder renders instead of raising. Worth adding given Series#trend's nil path currently has no test coverage at all.

3. Inconsistent with the existing convention for this exact situation. The codebase already has an established pattern for "no trend data yet": app/views/pages/dashboard/_net_worth_chart.html.erb:7 (if series.trend.present? / t(".data_not_available")) and app/views/holdings/_holding.html.erb:47-52 (if holding.trend / t(".no_cost_basis")) both guard on a possibly-nil trend and use an i18n-backed message via t(). This PR instead hardcodes a bare "–" string with no locale key in both files (violates the project's "all user-facing strings must use i18n" convention), and duplicates the identical if/else block across two files rather than extracting a shared partial (e.g. along the lines of shared/_trend_change). Worth aligning with the existing pattern rather than introducing a third, inconsistent one.

Recommend addressing 2 and 3 (and getting a straight answer on 1) before merging.


Generated by Claude Code

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