Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ Skip running checks which aren't relevant to your changes. However, at the very
- **Auth**: `ensure_authenticated!` for APIs, token via `Authorization` header or `?api_key=`
- **CSS**: Using Tailwind CSS, no inline styles, use utility classes. We define some custom classes in `config/tailwind.config.js` and `app/assets/tailwind/application.css`.

## 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.

Comment on lines +58 to +61
## Inertia Components

On Inertia pages, use the `<Button />` component for buttons, not `<button>` tags.
Expand Down
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ gem "thruster", require: false
# For query count tracking
gem "query_count"

# `counter_cache` on steroids
gem "counter_culture"

# Compact request logging
gem "lograge"

Expand Down
4 changes: 4 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ GEM
zeitwerk (>= 2.5.0)
concurrent-ruby (1.3.6)
connection_pool (3.0.2)
counter_culture (3.13.1)
activerecord (>= 4.2)
activesupport (>= 4.2)
countries (8.1.0)
unaccent (~> 0.3)
crack (1.0.1)
Expand Down Expand Up @@ -667,6 +670,7 @@ DEPENDENCIES
bullet
capybara
cloudflare-rails
counter_culture
countries
debug
doorkeeper (~> 5.8)
Expand Down
2 changes: 2 additions & 0 deletions app/controllers/admin/account_merger_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ def perform_merge(older_user, newer_user)
ActiveRecord::Base.transaction do
# 1. Move heartbeats from newer to older
heartbeat_count = Heartbeat.where(user_id: newer_user.id).update_all(user_id: older_user.id)
Heartbeat.adjust_active_count_for(older_user.id, heartbeat_count) if heartbeat_count.positive?
Heartbeat.adjust_active_count_for(newer_user.id, -heartbeat_count) if heartbeat_count.positive?
results << "#{heartbeat_count} heartbeats moved"

# 2. Transfer API keys from newer to older
Expand Down
6 changes: 5 additions & 1 deletion app/controllers/concerns/api/admin/v1/user_utilities.rb
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,7 @@ def user_heartbeats
query = query.where(editor: editor) if editor.present?
query = query.where(machine: machine) if machine.present?

total_count = query.count
total_count = unfiltered_heartbeats_request? ? user.active_heartbeats_count_or_count : query.count
source_types = Heartbeat.source_types.invert
heartbeats = query.order(time: :asc).limit(limit).offset(offset).pluck(*HEARTBEAT_RESPONSE_COLUMNS).map do |id, time, lineno, cursorpos, is_write, project, language, entity, branch, category, editor, machine, user_agent, ip_address, lines, source_type|
{
Expand Down Expand Up @@ -558,6 +558,10 @@ def find_user_by_id
nil
end

def unfiltered_heartbeats_request?
params.values_at(:start_date, :end_date, :project, :language, :entity, :editor, :machine).all?(&:blank?)
end

def parse_date_param
date_param = params[:date]
return Date.current if date_param.blank?
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/settings/imports_exports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def section_props
{
data_export: InertiaRails.defer {
{
total_heartbeats: number_with_delimiter(@user.heartbeats.count),
total_heartbeats: number_with_delimiter(@user.active_heartbeats_count_or_count),
total_coding_time: @user.heartbeats.duration_simple,
heartbeats_last_7_days: number_with_delimiter(@user.heartbeats.where("time >= ?", 7.days.ago.to_f).count),
is_restricted: (@user.trust_level == "red")
Expand Down
4 changes: 3 additions & 1 deletion app/jobs/one_time/transfer_user_data_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ def transfer_api_keys
end

def transfer_heartbeats
Heartbeat.where(user_id: @source_user_id).update_all(user_id: @target_user_id)
heartbeat_count = Heartbeat.where(user_id: @source_user_id).update_all(user_id: @target_user_id)
Heartbeat.adjust_active_count_for(@target_user_id, heartbeat_count) if heartbeat_count.positive?
Heartbeat.adjust_active_count_for(@source_user_id, -heartbeat_count) if heartbeat_count.positive?
end

def source_user
Expand Down
9 changes: 9 additions & 0 deletions app/models/heartbeat.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ class Heartbeat < ApplicationRecord
self.inheritance_column = nil

belongs_to :user
counter_culture :user, column_name: proc { |heartbeat| heartbeat.deleted_at.nil? ? "active_heartbeats_count" : nil }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 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.


validates :time, presence: true

Expand All @@ -111,15 +112,23 @@ def self.indexed_attributes
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
Comment on lines 114 to 126

def self.adjust_active_count_for(user_id, amount)
User.where(id: user_id).update_all([ "active_heartbeats_count = COALESCE(active_heartbeats_count, 0) + ?", amount ])
end

private

def set_fields_hash!
Expand Down
12 changes: 12 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class User < ApplicationRecord

after_create :subscribe_to_default_lists
after_create_commit :schedule_onboarding_check_in_email
before_create :initialize_active_heartbeats_count
before_validation :normalize_username
encrypts :slack_access_token, :github_access_token, :hca_access_token

Expand Down Expand Up @@ -390,6 +391,12 @@ def most_recent_direct_entry_heartbeat
heartbeats.where(source_type: :direct_entry).order(time: :desc).first
end

def active_heartbeats_count_or_count
return active_heartbeats_count if active_heartbeats_count_backfilled?

heartbeats.count
end
Comment on lines +394 to +398
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 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)

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.


def create_email_signin_token(continue_param: nil)
sign_in_tokens.create!(auth_type: :email, continue_param: continue_param)
end
Expand Down Expand Up @@ -438,6 +445,11 @@ def subscribe_to_default_lists
subscribe("weekly_summary")
end

def initialize_active_heartbeats_count
self.active_heartbeats_count = 0 if active_heartbeats_count.nil?
self.active_heartbeats_count_backfilled = true
end

def schedule_onboarding_check_in_email
OnboardingCheckInEmailJob.set(wait: 1.week).perform_later(id)
end
Expand Down
3 changes: 2 additions & 1 deletion app/services/anonymize_user_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ def destroy_associated_records
user.goals.destroy_all

# tombstone
Heartbeat.unscoped.where(user_id: user.id, deleted_at: nil).update_all(deleted_at: Time.current)
tombstoned_heartbeats_count = Heartbeat.unscoped.where(user_id: user.id, deleted_at: nil).update_all(deleted_at: Time.current)
Heartbeat.adjust_active_count_for(user.id, -tombstoned_heartbeats_count) if tombstoned_heartbeats_count.positive?

user.access_grants.destroy_all
user.access_tokens.destroy_all
Expand Down
3 changes: 2 additions & 1 deletion app/services/heartbeat_ingest.rb
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ def persist_direct_heartbeat(attrs)
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? ]
Comment on lines 121 to 125
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 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.

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.

end

Expand Down Expand Up @@ -196,7 +197,7 @@ def flush_import_batch(seen_hashes)

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 parse_user_agent(user_agent)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class AddActiveHeartbeatsCountToUsers < ActiveRecord::Migration[8.1]
def change
add_column :users, :active_heartbeats_count, :bigint
add_column :users, :active_heartbeats_count_backfilled, :boolean, null: false, default: false
end
end
4 changes: 3 additions & 1 deletion db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.