Clean up DashboardRollup code

[skyfallwastaken]

DashboardRollup was hastily put together to address Hackatime's perf issues. It works! However, the code is... not great - this PR hopefully fixes that.

[greptile-apps]

Greptile Summary

This PR replaces the 519-line DashboardData controller concern and the standalone DashboardRollupRefreshService with a single DashboardSnapshot service object that handles both reading (via call) and writing rollup records (via persist_rollups!). The controller and job are correspondingly thinned out, and a solid suite of service-level tests is added.

• DashboardSnapshot#call assembles filterable data, activity graph, and today-stats, pulling from rollup rows when available and falling back to live queries; a sources hash on the result exposes which path was taken, which the new tests verify.
• persist_rollups! consolidates the former service's logic and is now called directly from DashboardRollupRefreshJob, eliminating the intermediate DashboardRollupRefreshService class.
• Controller gains initial_dashboard_stats_prop to serve pre-built stats synchronously when a valid rollup exists, falling back to Inertia's deferred prop otherwise.

Confidence Score: 4/5

Safe to merge — the refactor is a faithful extraction of the existing concern into a service object with no behavioral changes to the rollup read/write paths, and the new tests cover all major branch combinations.

All findings are style and code-quality observations (unconventional method naming, a redundant cache lookup in filtered_scope, unused fields computed inside live_raw_snapshot on the read path, and two overly long predicate lines). None represent wrong data, broken contracts, or regressions in the rollup logic.

app/services/dashboard_snapshot.rb warrants a second look for the product_* naming, the filtered_scope / live_raw_filter_options redundancy, and the long single-line predicates in today_stats_rollup_valid? and activity_graph_rollup_valid?.

Important Files Changed

Filename	Overview
app/services/dashboard_snapshot.rb	New service object consolidating DashboardData concern + DashboardRollupRefreshService; contains the product_* naming oddity, a redundant live-filter-options fetch in filtered_scope, and two very long predicate lines.
app/controllers/static_pages_controller.rb	Controller slimmed down by delegating all dashboard data assembly to DashboardSnapshot; initial_dashboard_stats_prop guard logic is clean and correct.
app/jobs/dashboard_rollup_refresh_job.rb	Job now calls DashboardSnapshot#persist_rollups! directly; concurrency guard and enqueue-dedup logic are unchanged and correct.
test/services/dashboard_snapshot_test.rb	New integration-style tests covering rollup-served data, stale rollup refresh scheduling, and fragment invalidation; good coverage of the happy and fallback paths.
test/services/dashboard_snapshot_rollup_persistence_test.rb	Tests persist_rollups! end-to-end; verifies total row, dimension rows, filter options payload, activity graph payload, and today-stats payload.
test/controllers/static_pages_controller_test.rb	Rewrites old controller tests to verify immediate vs. deferred dashboard_stats prop delivery and validates weekly_project_stats and grouped duration values.

Sequence Diagram

sequenceDiagram
    participant C as StaticPagesController
    participant DS as DashboardSnapshot
    participant DR as DashboardRollup (DB)
    participant Cache as Rails.cache
    participant Job as DashboardRollupRefreshJob

    C->>DS: new(user:, params:).call
    DS->>DR: load all rows for user
    DS->>DS: raw_filter_options_for_read
    alt rollup rows present and valid
        DS-->>DS: use rollup filter options
    else
        DS->>Cache: fetch_multi (filter option keys)
        Cache-->>DS: live filter options
    end
    DS->>DS: aggregate_raw_for_read
    alt rollup eligible and total row present
        DS-->>DS: build Raw from rollup rows
        alt rollup stale
            DS->>Job: schedule_for(user_id, wait: 0s)
        end
    else
        DS->>Cache: fetch(live_aggregate_cache_key)
        Cache-->>DS: live_raw_snapshot result
    end
    DS->>DS: activity_graph_raw_for_read
    DS->>DS: today_stats_raw_for_read
    DS-->>C: Result(filterable_dashboard_data, activity_graph, today_stats, sources)

    Note over C,Job: Background refresh path
    Job->>DS: new(user:).persist_rollups!
    DS->>DS: live_raw_snapshot (full heartbeats)
    DS->>DR: transaction: delete_all + insert_all!
    DS->>DR: clear_dirty(user_id)

Loading

Prompt To Fix All With AI

Fix the following 5 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 5
app/services/dashboard_snapshot.rb:55
**Unconventional `product_*` method naming**

`product_filterable_dashboard_data`, `product_activity_graph`, and `product_today_stats` read as noun phrases ("the product of X") rather than verbs. The conventional Ruby pattern for builder/factory methods is `build_*` or `produce_*`. With three methods following this pattern it's likely intentional, but it's unusual enough to trip up future readers—`produce_*` or `build_*` would communicate intent more clearly.

### Issue 2 of 5
app/services/dashboard_snapshot.rb:258-275
**`filtered_scope` re-fetches live filter options independently**

`filtered_scope` always calls `live_raw_filter_options` internally, ignoring the `raw_filter_options` that was already resolved and passed into `aggregate_raw_for_read` from `call`. While this is benign in practice—`live_raw_filter_options` is backed by `Rails.cache.fetch_multi`, so a second call is just a cache lookup—it means `filtered_scope` can never benefit from rollup-sourced filter options even if a caller already holds them, and it makes the data flow harder to follow. Accepting `raw_filter_options` as a parameter would make the dependency explicit.

### Issue 3 of 5
app/services/dashboard_snapshot.rb:217-231
**`live_raw_snapshot` computes `activity_graph` and `today_stats` that are discarded in the read path**

`live_raw_snapshot` always populates `activity_graph` and `today_stats` on the returned `Raw` struct. When called from `persist_rollups!` those fields are needed. But when called from `aggregate_raw_for_read`, the `call` method reads activity-graph and today-stats via `activity_graph_raw_for_read` / `today_stats_raw_for_read` instead—never touching `raw.activity_graph` or `raw.today_stats`. Not a correctness bug (both sub-paths are themselves cached), but it makes `live_raw_snapshot` do more work than its callers need and obscures the intent.

### Issue 4 of 5
app/services/dashboard_snapshot.rb:495
The `today_stats_rollup_valid?` predicate is a single ~170-character line, making it hard to scan and diff. Breaking the conditions across lines improves readability.

```suggestion
    payload["timezone"] == @user.timezone &&
      payload["today_date"] == today_date &&
      payload.key?("todays_duration_seconds") &&
      payload["todays_language_categories"].is_a?(Array) &&
      payload["todays_editor_keys"].is_a?(Array)
```

### Issue 5 of 5
app/services/dashboard_snapshot.rb:488
`activity_graph_rollup_valid?` is also a single long line. Breaking conditions across lines improves readability and consistency.

```suggestion
    payload["timezone"] == @user.timezone &&
      payload["start_date"] == start_date &&
      payload["end_date"] == end_date &&
      payload["duration_by_date"].is_a?(Hash)
```

_{Reviews (1): Last reviewed commit: "Clean up `DashboardRollup` code" | Re-trigger Greptile}

[Copilot]

Pull request overview

This PR refactors the homepage/dashboard stats pipeline by extracting the former DashboardData concern + DashboardRollupRefreshService logic into a dedicated DashboardSnapshot service that can both (a) serve dashboard payloads and (b) persist/refresh DashboardRollup rows.

Changes:

• Introduces DashboardSnapshot as the single entry point for computing dashboard payloads (live vs rollup-backed) and for persisting rollups.
• Replaces DashboardRollupRefreshService usage in the refresh job and controllers with DashboardSnapshot#persist_rollups! / #call.
• Rehomes/rewrites test coverage from the old controller concern harness into service-focused tests.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
app/services/dashboard_snapshot.rb	New service encapsulating dashboard snapshot computation, rollup reads, and rollup persistence.
app/services/dashboard_rollup_refresh_service.rb	Removed; persistence logic is now in DashboardSnapshot#persist_rollups!.
app/controllers/static_pages_controller.rb	Stops including DashboardData; uses DashboardSnapshot and simplifies initial rollup gating.
app/controllers/concerns/dashboard_data.rb	Removed; functionality migrated into DashboardSnapshot.
app/jobs/dashboard_rollup_refresh_job.rb	Uses DashboardSnapshot#persist_rollups! instead of the removed refresh service.
test/services/dashboard_snapshot_test.rb	New service-level tests for rollup vs live behavior, staleness, and fragment validation.
test/services/dashboard_snapshot_rollup_persistence_test.rb	Renamed/updated test to validate persisted rollup content from DashboardSnapshot#persist_rollups!.
test/controllers/static_pages_controller_test.rb	Updated to use DashboardSnapshot#persist_rollups! in setup.
test/controllers/concerns/dashboard_data_test.rb	Removed along with the DashboardData concern.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.