Add counter_culture to heartbeats#1337
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a cached “active heartbeats” counter on users to avoid expensive COUNT(*) queries over large heartbeats tables, aiming to speed up endpoints/UI that show all-time heartbeat totals.
Changes:
- Add
counter_cultureand a cachedusers.active_heartbeats_count(+active_heartbeats_count_backfilledflag). - Maintain the counter manually in bulk-write paths (
insert_all/update_all/update_column) and switch some reads toUser#active_heartbeats_count_or_count. - Document the intended usage and caveats for the cached counter.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Gemfile | Adds counter_culture dependency. |
| Gemfile.lock | Locks counter_culture gem version. |
| db/schema.rb | Reflects new users columns for cached count + backfill flag. |
| db/migrate/20260521192204_add_active_heartbeats_count_to_users.rb | Adds active_heartbeats_count and active_heartbeats_count_backfilled columns. |
| app/services/heartbeat_ingest.rb | Updates cached count for direct ingest insert + import insert_all. |
| app/services/anonymize_user_service.rb | Updates cached count when tombstoning heartbeats via update_all. |
| app/models/user.rb | Adds active_heartbeats_count_or_count and initializes count for new users. |
| app/models/heartbeat.rb | Adds counter_culture, plus manual counter adjustments for soft delete/restore and a helper updater. |
| app/jobs/one_time/transfer_user_data_job.rb | Adjusts cached counts when moving heartbeats between users via update_all. |
| app/controllers/settings/imports_exports_controller.rb | Uses cached count (with fallback) for total heartbeat display. |
| app/controllers/concerns/api/admin/v1/user_utilities.rb | Uses cached count (with fallback) for unfiltered total count in admin heartbeats API. |
| app/controllers/admin/account_merger_controller.rb | Adjusts cached counts when merging/moving heartbeats via update_all. |
| AGENTS.md | Documents how/when to use and maintain the cached counter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## Heartbeat Counters | ||
|
|
||
| `users.active_heartbeats_count` is a cached count of non-deleted heartbeats. Use it only through `User#active_heartbeats_count_or_count` so users that have not been backfilled still fall back to an exact count. Heartbeat bulk writes (`insert_all`, `update_all`, soft-delete/anonymization/merge paths) bypass callbacks, so update the counter explicitly when changing active heartbeat rows outside normal ActiveRecord create/update flows. | ||
|
|
| persisted = if result.any? | ||
| Heartbeat.new(result.first) | ||
| else | ||
| @user.heartbeats.find_by!(fields_hash: fields_hash) | ||
| end | ||
|
|
||
| self.class.schedule_rollup_refresh(user: @user) if result.any? && @schedule_rollup_refresh | ||
| Heartbeat.adjust_active_count_for(@user.id, 1) if result.any? | ||
| [ persisted, !result.any? ] |
| def flush_import_batch(seen_hashes) | ||
| return 0 if seen_hashes.empty? | ||
|
|
||
| timestamp = Time.current | ||
| records = seen_hashes.values.map do |record| | ||
| record.merge(created_at: timestamp, updated_at: timestamp) | ||
| end | ||
|
|
||
| ActiveRecord::Base.logger.silence do | ||
| Heartbeat.insert_all(records, unique_by: [ :fields_hash ]).length | ||
| end | ||
| end.tap { |persisted_count| Heartbeat.adjust_active_count_for(@user.id, persisted_count) if persisted_count.positive? } | ||
| end |
| def soft_delete | ||
| was_active = deleted_at.nil? | ||
| update_column(:deleted_at, Time.current) | ||
| self.class.adjust_active_count_for(user_id, -1) if was_active | ||
| DashboardRollupRefreshJob.schedule_for(user_id) | ||
| end | ||
|
|
||
| def restore | ||
| was_deleted = deleted_at.present? | ||
| update_column(:deleted_at, nil) | ||
| self.class.adjust_active_count_for(user_id, 1) if was_deleted | ||
| DashboardRollupRefreshJob.schedule_for(user_id) | ||
| end |
Greptile SummaryThis PR adds a
Confidence Score: 3/5Safe for day-to-day counter maintenance, but the fix_counts reconciliation path will produce wrong results until the column_names hash is added. The manual counter adjustments across all write paths are internally consistent and the backfill-guard fallback prevents stale reads. However, the counter_culture declaration is missing the column_names hash required when column_name is a proc — running Heartbeat.counter_culture_fix_counts today would count soft-deleted heartbeats and silently overwrite correct counters with inflated values for every user. app/models/heartbeat.rb needs the column_names hash to make fix_counts safe to run. No test files were changed, leaving counter arithmetic in merge, transfer, and soft-delete paths unverified. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Heartbeat Write Event] --> B{Path}
B -->|Direct ingest - Heartbeat.insert| C[SQL INSERT bypasses callbacks]
B -->|Bulk import - Heartbeat.insert_all| D[SQL INSERT ALL bypasses callbacks]
B -->|soft_delete / restore - update_column| E[SQL UPDATE bypasses callbacks]
B -->|Merge / Transfer / Anonymize - update_all| F[SQL UPDATE ALL bypasses callbacks]
C --> G[Manual: adjust_active_count_for +1]
D --> H[Manual: adjust_active_count_for +N via .tap]
E --> I[Manual: adjust_active_count_for plus or minus 1]
F --> J[Manual: adjust_active_count_for plus or minus N]
G & H & I & J --> K[users.active_heartbeats_count]
K --> L{Read path}
L -->|backfilled?| M[Return active_heartbeats_count - fast cached read]
L -->|not backfilled| N[Return heartbeats.count - live SQL COUNT]
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
app/models/heartbeat.rb:90
**`fix_counts` reconciliation broken without `column_names` hash**
The `counter_culture` gem's `fix_counts` method (used to reconcile stale counters) requires a `column_names` hash when `column_name` is a proc. Without it, `Heartbeat.counter_culture_fix_counts` doesn't know to apply a `WHERE deleted_at IS NULL` condition, so it counts *all* heartbeats (including soft-deleted ones) and would silently overwrite `active_heartbeats_count` with an inflated total for every user. From the gem's README: "Manually populating counter caches with dynamic column names requires additional configuration." The fix is to add a `column_names` entry:
```ruby
counter_culture :user,
column_name: proc { |heartbeat| heartbeat.deleted_at.nil? ? "active_heartbeats_count" : nil },
column_names: { ["heartbeats.deleted_at IS NULL"] => "active_heartbeats_count" }
```
### Issue 2 of 3
app/services/heartbeat_ingest.rb:121-125
**Dual counter-update design risk**
`counter_culture`'s after-create callbacks never fire here because SQL `insert` bypasses AR callbacks, so the only source of truth for the counter update is this manual call. Any future caller that creates a Heartbeat via `Heartbeat.create(...)` would trigger `counter_culture` automatically *and* potentially also call `adjust_active_count_for`, causing a double-increment. It's worth adding a comment or guard to prevent that path.
### Issue 3 of 3
app/models/user.rb:394-398
**No tests for the new counter logic**
The PR adds `active_heartbeats_count_or_count`, `initialize_active_heartbeats_count`, `adjust_active_count_for`, and the conditional `counter_culture` hook — all load-bearing for correctness and performance. The repository instruction requires tests for new functionality. None of the changed files are spec/test files, so there is no coverage for the backfill-fallback branch, the counter-adjustment arithmetic in merge/transfer paths, or the soft-delete/restore symmetry.
Reviews (1): Last reviewed commit: "Add `counter_culture` to heartbeats" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| self.inheritance_column = nil | ||
|
|
||
| belongs_to :user | ||
| counter_culture :user, column_name: proc { |heartbeat| heartbeat.deleted_at.nil? ? "active_heartbeats_count" : nil } |
There was a problem hiding this comment.
fix_counts reconciliation broken without column_names hash
The counter_culture gem's fix_counts method (used to reconcile stale counters) requires a column_names hash when column_name is a proc. Without it, Heartbeat.counter_culture_fix_counts doesn't know to apply a WHERE deleted_at IS NULL condition, so it counts all heartbeats (including soft-deleted ones) and would silently overwrite active_heartbeats_count with an inflated total for every user. From the gem's README: "Manually populating counter caches with dynamic column names requires additional configuration." The fix is to add a column_names entry:
counter_culture :user,
column_name: proc { |heartbeat| heartbeat.deleted_at.nil? ? "active_heartbeats_count" : nil },
column_names: { ["heartbeats.deleted_at IS NULL"] => "active_heartbeats_count" }Prompt To Fix With AI
This is a comment left during a code review.
Path: app/models/heartbeat.rb
Line: 90
Comment:
**`fix_counts` reconciliation broken without `column_names` hash**
The `counter_culture` gem's `fix_counts` method (used to reconcile stale counters) requires a `column_names` hash when `column_name` is a proc. Without it, `Heartbeat.counter_culture_fix_counts` doesn't know to apply a `WHERE deleted_at IS NULL` condition, so it counts *all* heartbeats (including soft-deleted ones) and would silently overwrite `active_heartbeats_count` with an inflated total for every user. From the gem's README: "Manually populating counter caches with dynamic column names requires additional configuration." The fix is to add a `column_names` entry:
```ruby
counter_culture :user,
column_name: proc { |heartbeat| heartbeat.deleted_at.nil? ? "active_heartbeats_count" : nil },
column_names: { ["heartbeats.deleted_at IS NULL"] => "active_heartbeats_count" }
```
How can I resolve this? If you propose a fix, please make it concise.| end | ||
|
|
||
| self.class.schedule_rollup_refresh(user: @user) if result.any? && @schedule_rollup_refresh | ||
| Heartbeat.adjust_active_count_for(@user.id, 1) if result.any? | ||
| [ persisted, !result.any? ] |
There was a problem hiding this comment.
counter_culture's after-create callbacks never fire here because SQL insert bypasses AR callbacks, so the only source of truth for the counter update is this manual call. Any future caller that creates a Heartbeat via Heartbeat.create(...) would trigger counter_culture automatically and potentially also call adjust_active_count_for, causing a double-increment. It's worth adding a comment or guard to prevent that path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/services/heartbeat_ingest.rb
Line: 121-125
Comment:
**Dual counter-update design risk**
`counter_culture`'s after-create callbacks never fire here because SQL `insert` bypasses AR callbacks, so the only source of truth for the counter update is this manual call. Any future caller that creates a Heartbeat via `Heartbeat.create(...)` would trigger `counter_culture` automatically *and* potentially also call `adjust_active_count_for`, causing a double-increment. It's worth adding a comment or guard to prevent that path.
How can I resolve this? If you propose a fix, please make it concise.| def active_heartbeats_count_or_count | ||
| return active_heartbeats_count if active_heartbeats_count_backfilled? | ||
|
|
||
| heartbeats.count | ||
| end |
There was a problem hiding this comment.
The PR adds active_heartbeats_count_or_count, initialize_active_heartbeats_count, adjust_active_count_for, and the conditional counter_culture hook — all load-bearing for correctness and performance. The repository instruction requires tests for new functionality. None of the changed files are spec/test files, so there is no coverage for the backfill-fallback branch, the counter-adjustment arithmetic in merge/transfer paths, or the soft-delete/restore symmetry.
Context Used: In the Hackatime repo, please ensure that the PR i... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/models/user.rb
Line: 394-398
Comment:
**No tests for the new counter logic**
The PR adds `active_heartbeats_count_or_count`, `initialize_active_heartbeats_count`, `adjust_active_count_for`, and the conditional `counter_culture` hook — all load-bearing for correctness and performance. The repository instruction requires tests for new functionality. None of the changed files are spec/test files, so there is no coverage for the backfill-fallback branch, the counter-adjustment arithmetic in merge/transfer paths, or the soft-delete/restore symmetry.
**Context Used:** In the Hackatime repo, please ensure that the PR i... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.
Summary of the problem
It takes ~300ms to find all-time heartbeat counts for larger (1m+ heartbeats) users due to the sheer amount of rows. This can be very slow.
Describe your changes
Adds
counter_culture, which lets us find counts much faster, at the expense of slightly slower writes (but the indexes take up most of the write time anyway).Screenshots / Media
N/A