0.32.0 #1748
0.32.0 #1748
Conversation
Bumps [importmap-rails](https://github.com/rails/importmap-rails) from 2.1.0 to 2.2.2. - [Release notes](https://github.com/rails/importmap-rails/releases) - [Commits](rails/importmap-rails@v2.1.0...v2.2.2) --- updated-dependencies: - dependency-name: importmap-rails dependency-version: 2.2.2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [foreman](https://github.com/ddollar/foreman) from 0.88.1 to 0.90.0. - [Changelog](https://github.com/ddollar/foreman/blob/main/Changelog.md) - [Commits](ddollar/foreman@v0.88.1...v0.90.0) --- updated-dependencies: - dependency-name: foreman dependency-version: 0.90.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [selenium-webdriver](https://github.com/SeleniumHQ/selenium) from 4.33.0 to 4.35.0. - [Release notes](https://github.com/SeleniumHQ/selenium/releases) - [Changelog](https://github.com/SeleniumHQ/selenium/blob/trunk/rb/CHANGES) - [Commits](SeleniumHQ/selenium@selenium-4.33.0...selenium-4.35.0) --- updated-dependencies: - dependency-name: selenium-webdriver dependency-version: 4.35.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
…river-4.35.0 Bump selenium-webdriver from 4.33.0 to 4.35.0
Bump foreman from 0.88.1 to 0.90.0
…ls-2.2.2 Bump importmap-rails from 2.1.0 to 2.2.2
Implement a new daily track generation job to replace the old cleanup…
|
Caution Review failedThe pull request is closed. WalkthroughBumps app version to 0.32.0; removes legacy incremental track jobs/services; adds Tracks::DailyGenerationJob and schedule; replaces some track-generation flows with parallel/Redis-backed counters; implements stat sharing (DB fields, model helpers, controllers, views, routes, API); adds hexagon grid backend and frontend; many controllers, caching, mailer, view and test updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Scheduler as Cron
participant DailyJob as Tracks::DailyGenerationJob
participant Users as User.active_or_trial
participant ParallelJob as Tracks::ParallelGeneratorJob
participant Exception as ExceptionReporter
Note right of DailyJob #E6F1FF: runs every 4 hours
Scheduler->>DailyJob: trigger perform
DailyJob->>Users: find_each active_or_trial
loop per user
DailyJob->>DailyJob: compute start_timestamp(user)
alt new points exist (>= start_timestamp)
DailyJob->>ParallelJob: perform_later(user.id, start_at:, end_at:, mode: 'daily')
else
DailyJob-->>DailyJob: skip user
end
alt error during per-user processing
DailyJob->>Exception: ExceptionReporter.call(error, 'Failed to generate daily tracks')
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build_and_push.yml (1)
89-99: Avoid tagging :latest (and :rc) on manual/branch builds.
Current logic adds :latest when not a prerelease even on workflow_dispatch/branch runs. Gate tags by event type.-# Add :rc tag for pre-releases -if [ "${{ github.event.release.prerelease }}" = "true" ]; then - TAGS="${TAGS},freikin/dawarich:rc" - # For RC builds, only use amd64 - PLATFORMS="linux/amd64" -fi - -# Add :latest tag only if release is not a pre-release -if [ "${{ github.event.release.prerelease }}" != "true" ]; then - TAGS="${TAGS},freikin/dawarich:latest" -fi +# Tag only on release events +if [ "$GITHUB_EVENT_NAME" = "release" ]; then + if [ "${{ github.event.release.prerelease }}" = "true" ]; then + TAGS="${TAGS},freikin/dawarich:rc" + PLATFORMS="linux/amd64" + else + TAGS="${TAGS},freikin/dawarich:latest" + fi +fi
🧹 Nitpick comments (5)
app/models/user.rb (1)
30-31: Prefer composing enum scopes for clarity and future-proofingUsing the enum-generated scopes reads cleaner and avoids hard-coding the mapping.
- scope :active_or_trial, -> { where(status: %i[active trial]) } + scope :active_or_trial, -> { active.or(trial) }Optional: if queries by status are frequent, consider an index on users.status.
spec/jobs/tracks/daily_generation_job_spec.rb (2)
56-62: Strengthen argument assertions for enqueued jobsAlso assert start_at/end_at types to lock interface and catch regressions.
- expect { described_class.perform_now }.to \ - have_enqueued_job(Tracks::ParallelGeneratorJob).with( - active_user.id, - hash_including(mode: 'daily') - ) + expect { described_class.perform_now }.to \ + have_enqueued_job(Tracks::ParallelGeneratorJob).with( + active_user.id, + hash_including(mode: 'daily', start_at: kind_of(Integer), end_at: kind_of(Integer)) + )- expect { described_class.perform_now }.to \ - have_enqueued_job(Tracks::ParallelGeneratorJob).with( - trial_user.id, - hash_including(mode: 'daily') - ) + expect { described_class.perform_now }.to \ + have_enqueued_job(Tracks::ParallelGeneratorJob).with( + trial_user.id, + hash_including(mode: 'daily', start_at: kind_of(Integer), end_at: kind_of(Integer)) + )Also applies to: 64-70
72-77: Optional: keep the fast path realisticAfter Point.destroy_all, consider syncing points_count to 0 for active_user and trial_user to keep the “quick skip” path realistic.
it 'does not enqueue jobs for users without new points' do Point.destroy_all + active_user.update!(points_count: 0) + trial_user.update!(points_count: 0) expect { described_class.perform_now }.not_to \ have_enqueued_job(Tracks::ParallelGeneratorJob) endapp/jobs/tracks/daily_generation_job.rb (1)
23-29: Avoid relying solely on counter caches for correctnesspoints_count can drift; consider replacing the prefilter with an exists? check, or combining them to keep it fast and correct on drift.
- User.active_or_trial.find_each do |user| - next if user.points_count.zero? + User.active_or_trial.find_each do |user| + next if user.points_count.zero? && !user.points.exists?spec/services/tracks/parallel_generator_spec.rb (1)
194-198: Job coordination delay checks are sensible, but consider freezing timeUsing be >= N.minutes.from_now is fine; adding travel_to(Time.current) would remove timing flakiness entirely.
+ travel_to(Time.current) do expect do generator.call end.to have_enqueued_job(Tracks::BoundaryResolverJob) .with(user.id, kind_of(String)) - end + end(Apply similarly to other delay specs.)
Also applies to: 202-206, 292-297, 300-304, 307-311
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.app_version(1 hunks).github/workflows/build_and_push.yml(1 hunks)CHANGELOG.md(1 hunks)app/jobs/tracks/cleanup_job.rb(0 hunks)app/jobs/tracks/create_job.rb(0 hunks)app/jobs/tracks/daily_generation_job.rb(1 hunks)app/jobs/tracks/incremental_check_job.rb(0 hunks)app/jobs/tracks/parallel_generator_job.rb(1 hunks)app/jobs/tracks/time_chunk_processor_job.rb(1 hunks)app/models/point.rb(2 hunks)app/models/user.rb(1 hunks)app/services/tracks/generator.rb(0 hunks)app/services/tracks/incremental_processor.rb(0 hunks)app/services/tracks/parallel_generator.rb(2 hunks)app/services/tracks/segmentation.rb(1 hunks)app/services/tracks/session_manager.rb(2 hunks)app/services/tracks/track_builder.rb(4 hunks)config/schedule.yml(1 hunks)db/data/20250704185707_create_tracks_from_points.rb(1 hunks)spec/jobs/tracks/cleanup_job_spec.rb(0 hunks)spec/jobs/tracks/create_job_spec.rb(0 hunks)spec/jobs/tracks/daily_generation_job_spec.rb(1 hunks)spec/jobs/tracks/incremental_check_job_spec.rb(0 hunks)spec/jobs/tracks/parallel_generator_job_spec.rb(1 hunks)spec/services/tracks/generator_spec.rb(0 hunks)spec/services/tracks/incremental_processor_spec.rb(0 hunks)spec/services/tracks/parallel_generator_spec.rb(6 hunks)
💤 Files with no reviewable changes (10)
- spec/jobs/tracks/create_job_spec.rb
- app/services/tracks/incremental_processor.rb
- spec/jobs/tracks/incremental_check_job_spec.rb
- spec/jobs/tracks/cleanup_job_spec.rb
- spec/services/tracks/generator_spec.rb
- app/jobs/tracks/incremental_check_job.rb
- app/services/tracks/generator.rb
- spec/services/tracks/incremental_processor_spec.rb
- app/jobs/tracks/cleanup_job.rb
- app/jobs/tracks/create_job.rb
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop with Rails extensions to lint Ruby code
Files:
app/services/tracks/track_builder.rbapp/services/tracks/segmentation.rbspec/jobs/tracks/daily_generation_job_spec.rbapp/jobs/tracks/daily_generation_job.rbapp/services/tracks/session_manager.rbapp/models/user.rbapp/jobs/tracks/parallel_generator_job.rbdb/data/20250704185707_create_tracks_from_points.rbapp/jobs/tracks/time_chunk_processor_job.rbspec/services/tracks/parallel_generator_spec.rbspec/jobs/tracks/parallel_generator_job_spec.rbapp/models/point.rbapp/services/tracks/parallel_generator.rb
app/{controllers,services}/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use Sidekiq for any potentially long-running operations instead of performing them inline
Files:
app/services/tracks/track_builder.rbapp/services/tracks/segmentation.rbapp/services/tracks/session_manager.rbapp/services/tracks/parallel_generator.rb
app/{models,queries}/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Leverage PostGIS (database-side) functions for geographic calculations instead of Ruby-based solutions
Files:
app/models/user.rbapp/models/point.rb
app/models/point.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use the
lonlatcolumn in thepointstable for geographic calculations
Files:
app/models/point.rb
🧠 Learnings (1)
📚 Learning: 2025-09-04T20:15:35.535Z
Learnt from: CR
PR: Freika/dawarich#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T20:15:35.535Z
Learning: Applies to app/models/point.rb : Use the `lonlat` column in the `points` table for geographic calculations
Applied to files:
app/services/tracks/track_builder.rb
🧬 Code graph analysis (5)
spec/jobs/tracks/daily_generation_job_spec.rb (1)
spec/services/visits/suggest_spec.rb (1)
enqueued_jobs(140-142)
app/jobs/tracks/daily_generation_job.rb (2)
app/jobs/tracks/parallel_generator_job.rb (2)
queue_as(5-21)perform(8-20)app/services/tracks/parallel_generator.rb (1)
call(19-38)
spec/services/tracks/parallel_generator_spec.rb (2)
app/services/tracks/parallel_generator.rb (1)
call(19-38)app/services/tracks/time_chunker.rb (1)
call(16-36)
app/models/point.rb (1)
app/serializers/api/photo_serializer.rb (1)
country(57-59)
app/services/tracks/parallel_generator.rb (1)
app/javascript/maps/helpers.js (1)
days(36-36)
🔇 Additional comments (17)
.app_version (1)
1-1: Version bump OK.
Matches PR title 0.31.1.app/jobs/tracks/time_chunk_processor_job.rb (1)
50-51: LGTM on simplification.
Inline increment keeps semantics and reads cleaner.db/data/20250704185707_create_tracks_from_points.rb (1)
5-8: Deprecation to no-op looks fine.
Safe to keep migration idempotent; irreversible down preserved.app/models/point.rb (1)
69-72: country_name fallback tweak OK.
Using self[:country] is equivalent and a bit simpler; no behavior change.config/schedule.yml (1)
38-41: Approve daily_track_generation_job schedule
Class Tracks::DailyGenerationJob is defined in app/jobs/tracks/daily_generation_job.rb (line 19) with queue_as :tracks; cron “0 */4 * * *” correctly triggers every 4 hours.app/services/tracks/segmentation.rb (1)
24-25: Docs alignment with new parallel flow — LGTMapp/jobs/tracks/parallel_generator_job.rb (1)
11-17: Inline instantiation + immediate call — LGTMNo behavior change; simpler and clear.
app/services/tracks/track_builder.rb (3)
28-29: Docs update to reference ParallelGenerator — LGTM
107-107: Speed cap constant formatting — LGTM
147-149: NotImplementedError message style change — LGTMThe class including this module (e.g., ParallelGenerator) provides user via attr_reader, satisfying this contract.
spec/jobs/tracks/parallel_generator_job_spec.rb (1)
129-132: Expectation style tweak — LGTMKeeps the assertion explicit and readable.
app/services/tracks/parallel_generator.rb (1)
141-143: Numeric literal readability — LGTMspec/jobs/tracks/daily_generation_job_spec.rb (1)
32-35: Good, broad coverage of primary flowsHappy path, inactive path isolation, and failure reporting assertions look solid.
Also applies to: 51-54, 79-96
app/jobs/tracks/daily_generation_job.rb (1)
3-18: Docs match intentNice, clear operational comments and rationale for the schedule and flow.
spec/services/tracks/parallel_generator_spec.rb (3)
83-86: RSpec block-form enqueued job assertions look goodSwitch to do/end improves readability; expectations remain equivalent.
Also applies to: 89-92, 111-114
136-144: Daily mode “preserves existing tracks” assertion aligns with the new flowGood update reflecting removal of daily cleanup; expectation is precise.
264-269: Chunk enqueuing coverage looks solidVerifies both count and exact params for chunk jobs; nice.
Also applies to: 272-276
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
spec/services/tracks/parallel_generator_spec.rb (2)
194-198: Nice: verifies resolver is enqueued with session argsConsider also asserting the queue name if your jobs use dedicated queues (e.g.,
.on_queue('tracks')) for stricter guarantees.Example:
- end.to have_enqueued_job(Tracks::BoundaryResolverJob) - .with(user.id, kind_of(String)) + end.to have_enqueued_job(Tracks::BoundaryResolverJob) + .with(user.id, kind_of(String)) + .on_queue('tracks')
265-269: Stub chunks closer to real shape to improve test fidelity
TimeChunkerlikely yields chunks with bothstart_timestampandend_timestamp(and possibly range bounds). Passing minimal hashes can let regressions slip. Consider stubbing with realistic fields.Apply this diff:
let(:chunks) do - [ - { chunk_id: 'chunk1', start_timestamp: 1.day.ago.to_i }, - { chunk_id: 'chunk2', start_timestamp: 2.days.ago.to_i } - ] + start1 = 1.day.ago.to_i + start2 = 2.days.ago.to_i + [ + { chunk_id: 'chunk1', start_timestamp: start1, end_timestamp: start1 + 1.hour }, + { chunk_id: 'chunk2', start_timestamp: start2, end_timestamp: start2 + 1.hour } + ] endAlternatively, spy on
generate_time_chunksand use its return to avoid hand-crafted shapes.app/services/tracks/session_manager.rb (1)
5-178: Class length exceeds RuboCop default (121 > 100). Consider extracting helpers.Split out counter helpers into a small module (e.g., Tracks::CounterStore) or move class methods into a separate concern to keep this service focused.
spec/services/tracks/session_manager_spec.rb (2)
139-145: Counter increment test—LGTM. Consider adding a concurrency stress test.Example additional spec:
it 'increments completed_chunks atomically under concurrency' do manager.create_session manager.mark_started(1000) threads = 20.times.map { Thread.new { 100.times { manager.increment_completed_chunks } } } threads.each(&:join) expect(manager.get_session_data['completed_chunks']).to eq(2000) end
46-51: Test title mismatches assertion
The example is titled “sets TTL on the cache entry” but only checks that the key exists. Either rename the test to reflect that it just creates a cache entry or add a TTL assertion. For example:it 'sets TTL on the cache entry' do manager.create_session(metadata) cache_key = manager.send(:cache_key) expect(Rails.cache.exist?(cache_key)).to be true ttl = Rails.cache.redis.with { |r| r.ttl(cache_key) } expect(ttl).to be > 0 endIf other environments might use a non-Redis store, wrap the TTL check in a conditional or
skipblock.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/jobs/tracks/daily_generation_job.rb(1 hunks)app/services/tracks/parallel_generator.rb(3 hunks)app/services/tracks/session_manager.rb(6 hunks)app/services/tracks/track_builder.rb(4 hunks)spec/jobs/tracks/daily_generation_job_spec.rb(1 hunks)spec/services/tracks/parallel_generator_spec.rb(7 hunks)spec/services/tracks/session_manager_spec.rb(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/jobs/tracks/daily_generation_job.rb
- app/services/tracks/track_builder.rb
- app/services/tracks/parallel_generator.rb
- spec/jobs/tracks/daily_generation_job_spec.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop with Rails extensions to lint Ruby code
Files:
spec/services/tracks/session_manager_spec.rbapp/services/tracks/session_manager.rbspec/services/tracks/parallel_generator_spec.rb
app/{controllers,services}/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use Sidekiq for any potentially long-running operations instead of performing them inline
Files:
app/services/tracks/session_manager.rb
🧬 Code graph analysis (3)
spec/services/tracks/session_manager_spec.rb (2)
app/services/tracks/session_manager.rb (8)
session_exists?(61-63)cleanup_session(129-134)create_session(17-37)update_session(40-47)increment_completed_chunks(75-80)increment_tracks_created(83-88)get_session_data(50-58)mark_failed(99-105)app/jobs/tracks/time_chunk_processor_job.rb (1)
session_exists?(30-36)
app/services/tracks/session_manager.rb (1)
app/jobs/tracks/time_chunk_processor_job.rb (1)
session_exists?(30-36)
spec/services/tracks/parallel_generator_spec.rb (3)
app/services/tracks/parallel_generator.rb (1)
call(19-38)app/services/tracks/time_chunker.rb (1)
call(16-36)app/services/tracks/track_builder.rb (1)
user(147-149)
🪛 RuboCop (1.76.1)
app/services/tracks/session_manager.rb
[convention] 5-178: Class has too many lines. [121/100]
(Metrics/ClassLength)
🔇 Additional comments (21)
spec/services/tracks/parallel_generator_spec.rb (4)
111-114: LGTM: correctly asserts no jobs enqueued when no chunksThis ensures the adapter queue remains empty for users without points.
136-144: Daily mode semantics validatedAsserting preservation of existing tracks in daily mode aligns with production behavior (only bulk cleans). Good coverage.
164-166: Good: time-range fixtures include end_atAdding
end_atmakes range-based cleanup assertions precise and avoids ambiguity in overlapping ranges.
272-276: LGTM: asserts one job per chunk
.exactly(2).timesmatches the two provided chunks—good signal.app/services/tracks/session_manager.rb (3)
54-57: Augmenting session with live counters looks good.
112-114: All-chunks-completed check is correct (>= handles over-increment edge).
124-126: Progress calculation is sound; 100% when total is 0 is acceptable.spec/services/tracks/session_manager_spec.rb (14)
29-44: Session creation defaults verified—nice coverage.
60-66: get_session_data basic behavior—LGTM.
85-94: update_session happy path—LGTM.
96-101: update_session returns false when missing—LGTM.
103-113: Preserves existing metadata on update—LGTM.
121-130: mark_started semantics—LGTM.
158-164: tracks_created default increment—LGTM.
166-172: tracks_created increment by amount—LGTM.
185-193: mark_completed test—LGTM.
201-207: mark_failed test—LGTM.
247-253: cleanup_session removal—LGTM.
259-269: .create_for_user behavior—LGTM.
271-277: Cache key scoping assertion—LGTM.
279-286: Cross-user isolation—LGTM.
| def increment_completed_chunks | ||
| session_data = get_session_data | ||
| return false unless session_data | ||
| return false unless session_exists? | ||
|
|
||
| new_completed = session_data['completed_chunks'] + 1 | ||
| update_session(completed_chunks: new_completed) | ||
| atomic_increment(counter_key('completed_chunks'), 1) | ||
| true | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refresh counter TTL on each increment to avoid mid-run expiry.
INCRBY doesn’t extend TTL; counters can expire during long runs, and a post-cleanup increment recreates keys without TTL. Update atomic_increment to pipeline INCRBY + EXPIRE (and fall back to Rails.cache.increment when Redis isn’t available).
Apply the atomic_increment diff in Lines 173-177 below.
Also applies to: 83-88
🤖 Prompt for AI Agents
In app/services/tracks/session_manager.rb around lines 75-80 and apply the
atomic_increment change at lines 173-177, the current INCRBY call doesn't
refresh the Redis key TTL causing counters to expire mid-run; update
atomic_increment to use a Redis pipelined INCRBY plus EXPIRE (set TTL to the
desired expiry) so each increment refreshes the TTL, and keep a fallback branch
that uses Rails.cache.increment when Redis is unavailable; ensure error
handling/logging remains and that both completed_chunks (lines 75-80) and the
similar counter code at 83-88 use the updated atomic_increment behavior.
|
|
||
| def counter_key(field) | ||
| "#{cache_key}:#{field}" | ||
| end | ||
|
|
||
| def counter_value(field) | ||
| Rails.cache.redis.with do |redis| | ||
| (redis.get(counter_key(field)) || 0).to_i | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify counter_value and remove hard Redis dependency.
- def counter_value(field)
- Rails.cache.redis.with do |redis|
- (redis.get(counter_key(field)) || 0).to_i
- end
- end
+ def counter_value(field)
+ Rails.cache.read(counter_key(field)).to_i
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def counter_key(field) | |
| "#{cache_key}:#{field}" | |
| end | |
| def counter_value(field) | |
| Rails.cache.redis.with do |redis| | |
| (redis.get(counter_key(field)) || 0).to_i | |
| end | |
| end | |
| def counter_key(field) | |
| "#{cache_key}:#{field}" | |
| end | |
| def counter_value(field) | |
| Rails.cache.read(counter_key(field)).to_i | |
| end |
🤖 Prompt for AI Agents
In app/services/tracks/session_manager.rb around lines 162 to 171, counter_value
currently opens a direct Redis connection via Rails.cache.redis.with; replace
this with a simple cache read to remove the hard Redis dependency. Implement
counter_value to return Rails.cache.read(counter_key(field)).to_i (or
Rails.cache.fetch(counter_key(field)) { 0 }.to_i) so it uses the configured
cache store, handles nil safely, and relies on the existing counter_key method
instead of calling Redis directly.
Feature/stats page
There was a problem hiding this comment.
Actionable comments posted: 46
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
app/jobs/users/mailer_sending_job.rb (1)
6-19: Narrow exception scope; avoid method-wide rescue that can mask real bugsCatching
ActiveRecord::RecordNotFoundat the method level may swallow unrelated AR lookups triggered downstream (e.g., inside a mailer). Preferfind_by+ early return and use the job’slogger(tagged with job context) instead ofRails.logger.def perform(user_id, email_type, **options) - user = User.find(user_id) + user = User.find_by(id: user_id) + unless user + logger.warn "User with ID #{user_id} not found. Skipping #{email_type} email." + return + end if trial_related_email?(email_type) && user.active? - Rails.logger.info "Skipping #{email_type} email for user #{user_id} - user is already subscribed" + logger.info "Skipping #{email_type} email for user #{user_id} - user is already subscribed" return end params = { user: user }.merge(options) UsersMailer.with(params).public_send(email_type).deliver_later -rescue ActiveRecord::RecordNotFound - Rails.logger.warn "User with ID #{user_id} not found. Skipping #{email_type} email." endspec/jobs/users/mailer_sending_job_spec.rb (1)
110-117: Fix spec description and assert that no mail is attempted for missing userThe example still claims it raises, but behavior now skips silently. Also assert no mailer calls occur.
- context 'when user is deleted' do - it 'raises ActiveRecord::RecordNotFound' do + context 'when user is deleted' do + it 'does not raise and does not attempt to send an email' do user.destroy - expect do - described_class.perform_now(user.id, 'welcome') - end.not_to raise_error + expect(UsersMailer).not_to receive(:with) + expect { described_class.perform_now(user.id, 'welcome') }.not_to raise_error end endspec/requests/api/v1/settings_spec.rb (1)
23-27: Add a regression check that sensitive keys aren’t echoed back.Since the controller permits API keys in settings, assert that responses use
safe_settingsand exclude secrets.Example test to append:
it 'does not expose secret keys in response' do patch "/api/v1/settings?api_key=#{api_key}", params: { settings: { immich_api_key: 'sekret', photoprism_api_key: 'sekret2' } } body = response.parsed_body['settings'] expect(body.keys).not_to include('immich_api_key', 'photoprism_api_key') endspec/requests/api/v1/subscriptions_spec.rb (1)
9-13: Avoid stubbing ENV as a constant; stub specific lookups instead.Replacing ENV globally can cause hard-to-debug side effects.
- before do - stub_const('ENV', ENV.to_h.merge('JWT_SECRET_KEY' => 'test_secret')) + before do + allow(ENV).to receive(:[]).and_call_original + allow(ENV).to receive(:[]).with('JWT_SECRET_KEY').and_return('test_secret') stub_request(:any, 'https://api.github.com/repos/Freika/dawarich/tags') .to_return(status: 200, body: '[{"name": "1.0.0"}]', headers: {}) endOptionally set
let(:jwt_secret) { 'test_secret' }and use it directly in token building.app/controllers/api/v1/settings_controller.rb (1)
17-22: Security: Don’t echo full settings (including API keys) back to clients.Success responses use
current_api_user.settings, which can includeimmich_api_key/photoprism_api_key. Returnsafe_settingsconsistently to avoid leaking secrets.- render json: { message: 'Settings updated', settings: current_api_user.settings, status: 'success' }, + render json: { message: 'Settings updated', settings: current_api_user.safe_settings, status: 'success' }, status: :ok @@ - render json: { message: 'Something went wrong', errors: current_api_user.errors.full_messages }, + render json: { message: 'Something went wrong', errors: current_api_user.errors.full_messages }, status: :unprocessable_contentFollow-up: add a spec asserting that secret keys are excluded (see suggested test in spec/requests/api/v1/settings_spec.rb).
app/controllers/api/v1/visits_controller.rb (2)
26-31: Avoid 500s on update: stop using save! and handle validation errors cleanly.
updatealways renders success andupdate_visitcallssave!, which will raise and bubble a 500 on validation failures. Prefer safe update with error response.Apply:
def update - visit = current_api_user.visits.find(params[:id]) - visit = update_visit(visit) - - render json: Api::VisitSerializer.new(visit).call + visit = current_api_user.visits.find(params[:id]) + if visit.update(visit_params.except(:latitude, :longitude)) + render json: Api::VisitSerializer.new(visit).call + else + render json: { error: 'Failed to update visit', errors: visit.errors.full_messages }, status: :unprocessable_content + end end @@ -def update_visit(visit) - visit_params.each do |key, value| - next if %w[latitude longitude].include?(key.to_s) - - visit[key] = value - visit.name = visit.place.name if visit_params[:place_id].present? - end - - visit.save! - - visit -end +def update_visit(_visit); end # no longer used; remove this method entirelyAdditionally, please delete
update_visitentirely to avoid future misuse.Also applies to: 107-118
33-38: Use strong params for merge_ids and enforce array shape.
params[:visit_ids]bypassesmerge_params; also ensure array and min length check is robust.-visit_ids = params[:visit_ids] +visit_ids = Array(merge_params[:visit_ids]).reject(&:blank?) if visit_ids.blank? || visit_ids.length < 2Also applies to: 99-105
spec/requests/settings/background_jobs_spec.rb (1)
53-55: Test has no expectations; it currently passes vacuously.The “redirects to imports page” example for photoprism doesn’t assert anything.
- it 'redirects to imports page' do - get settings_background_jobs_url, params: { job_name: 'start_photoprism_import' } - end + it 'redirects to imports page' do + get settings_background_jobs_url, params: { job_name: 'start_photoprism_import' } + expect(response).to redirect_to(imports_url) + endapp/controllers/imports_controller.rb (1)
101-102: Avoid logging signed IDs (potentially sensitive).Even truncated, signed IDs are secrets. Log a hash/fingerprint instead.
-Rails.logger.debug "Creating import from signed ID: #{signed_id[0..20]}..." +Rails.logger.debug "Creating import from signed ID (sha256): #{Digest::SHA256.hexdigest(signed_id)[0..10]}..."
♻️ Duplicate comments (1)
CHANGELOG.md (1)
11-13: Fix workflow/docs phrasing (repeat of earlier suggestion)
- Tense/grammar: “Runs every 4 hours.”
- Clarify patch vs minor builds.
Apply:
- - A cron job to generate daily tracks for users with new points since their last track generation. Being run every 4 hours. + - A cron job to generate daily tracks for users with new points since their last track generation. Runs every 4 hours. @@ - - Minor versions are now being built only for amd64 architecture to speed up the build process. + - Patch versions (x.y.z where z > 0) are now built only for amd64 to speed up the build process; minor versions (x.y.0) continue to build for all supported platforms.Also applies to: 17-17
🧹 Nitpick comments (67)
spec/models/user_spec.rb (2)
332-337: Make the spec deterministic by scopingTime.zone.Avoid dependence on global test env timezone; assert against a known zone.
Apply this diff:
-describe '#timezone' do - it 'returns the app timezone' do - expect(user.timezone).to eq(Time.zone.name) - end -end +describe '#timezone' do + it 'reflects the current Time.zone' do + Time.use_zone('UTC') do + expect(user.timezone).to eq('UTC') + end + end +end
332-337: Add an integration test covering timezone in rendered UI.Per our guideline for location/timezone features, include a system/request spec asserting timestamps are presented in the app (or user) timezone.
Want me to draft a
spec/system/...example that creates a record with a known timestamp and verifies the formatted time matchesTime.use_zone('UTC')?app/jobs/users/mailer_sending_job.rb (3)
14-16: Guard against unknown email types to avoid noisy job failuresIf
email_typeis invalid,public_sendwill raiseNoMethodError. Consider whitelisting or arespond_to?guard that logs and returns.- params = { user: user }.merge(options) + params = { user: user }.merge(options) + unless UsersMailer.respond_to?(email_type.to_s) + logger.warn "Unknown email type '#{email_type}' for user #{user.id}; skipping." + return + end
14-14: Minor: prefer keyword merge for clarity and speedRuby 2.7+ allows keyword splat for hashes with symbol keys.
- params = { user: user }.merge(options) + params = { user: user, **options }
21-25: Minor: avoid reallocating the list each callExtract the list to a frozen constant and use symbols to reduce allocations.
class Users::MailerSendingJob < ApplicationJob queue_as :mailers + TRIAL_EMAIL_TYPES = %i[trial_expires_soon trial_expired].freeze @@ - def trial_related_email?(email_type) - %w[trial_expires_soon trial_expired].include?(email_type.to_s) - end + def trial_related_email?(email_type) + TRIAL_EMAIL_TYPES.include?(email_type.to_sym) + endspec/jobs/users/mailer_sending_job_spec.rb (1)
110-117: Optional: verify warning is logged for missing userAdds a tight assertion to document the intended log message.
it 'does not raise and does not attempt to send an email' do user.destroy - expect(UsersMailer).not_to receive(:with) - expect { described_class.perform_now(user.id, 'welcome') }.not_to raise_error + expect(UsersMailer).not_to receive(:with) + expect(Rails.logger).to receive(:warn).with(/User with ID #{user.id} not found\. Skipping welcome email\./) + expect { described_class.perform_now(user.id, 'welcome') }.not_to raise_error endapp/views/devise/registrations/_api_key.html.erb (1)
5-8: Avoid nestedtags; consider shoulder‑surfing for QR
- HTML: don’t nest
<p>inside<p>.- UX: optional “Show QR” toggle to reduce accidental exposure.
Apply within this hunk:
- <p class='py-2'> + <div class='py-2'> Or you can scan it in your Dawarich iOS app: <%= api_key_qr_code(current_user) %> - </p> + </div>Outside this hunk, also change outer wrapper to a
div:-<p class="py-6"> +<div class="py-6"> ... -</p> +</div>app/views/stats/_year.html.erb (1)
13-19: Palette addition looks good; tweak axis label and DRY palette
- Suggest
xtitle: 'Months'(data is per-month).- Consider moving the colors array to a helper constant to reuse across charts.
Example outside this hunk:
- xtitle: 'Days', + xtitle: 'Months',app/services/users/import_data/stats.rb (2)
63-69: Preserve incoming sharing_uuid only if desired; otherwise always regenerateIf the product intent is to retain share links across imports, keep incoming
sharing_uuidwhen present/valid; else your current code correctly re-keys everything.Apply to prefer-preserve behavior:
- attributes = stat_data.except('created_at', 'updated_at', 'sharing_uuid') + attributes = stat_data.except('created_at', 'updated_at') ... - attributes['sharing_uuid'] = SecureRandom.uuid + attributes['sharing_uuid'] ||= SecureRandom.uuidIf you want to always rotate links, keep current code.
63-69: Enforce DB-level guarantees for sharing_uuidConsider
NOT NULL+ DB default (gen_random_uuid()) onstats.sharing_uuidto prevent nulls and reduce app-side work. Requires enabling pgcrypto and a follow-up migration.Follow-up migration (example):
class EnforceUuidOnStats < ActiveRecord::Migration[8.0] disable_ddl_transaction! def up enable_extension 'pgcrypto' unless extension_enabled?('pgcrypto') change_column_default :stats, :sharing_uuid, -> { 'gen_random_uuid()' } execute <<~SQL UPDATE stats SET sharing_uuid = gen_random_uuid() WHERE sharing_uuid IS NULL; SQL change_column_null :stats, :sharing_uuid, false end def down change_column_null :stats, :sharing_uuid, true change_column_default :stats, :sharing_uuid, nil end endGemfile (2)
32-32: Pin rails_icons to a version.Lock to a known-good minor to avoid API/markup changes unexpectedly altering views.
-gem 'rails_icons' +gem 'rails_icons', '~> 0.5' # pick the current minor you tested
43-43: Drop redundant sentry-ruby.sentry-rails already depends on sentry-ruby; keeping both widens the surface and can complicate upgrades.
-gem 'sentry-ruby'db/schema.rb (2)
223-228: Sharing columns: good defaults; confirm UUID uniqueness semantics.Unique index allows multiple NULLs (Postgres). If you require at most one NULL (rare), use UNIQUE NULLS NOT DISTINCT (PG ≥15) or a partial unique index WHERE sharing_uuid IS NOT NULL.
Proposed partial unique index migration:
+add_index :stats, :sharing_uuid, unique: true, where: "sharing_uuid IS NOT NULL", name: :index_stats_on_sharing_uuid
208-210: Consider partial index to shrink size and improve selectivity.If queries filter country_name when present, a partial index avoids indexing NULLs.
-remove_index :points, name: :idx_points_user_country_name -add_index :points, %i[user_id country_name], name: :idx_points_user_country_name, where: "country_name IS NOT NULL"config/initializers/rails_icons.rb (1)
3-14: Initializer looks fine; document chosen library.Optional: add a short comment on why 'lucide' to guide future changes.
app/views/shared/_navbar.html.erb (1)
119-121: Admin badge: add accessible label.Expose a textual label in addition to the icon.
- <span class='tooltip tooltip-bottom' data-tip="You're an admin, Harry!"> - <%= icon 'star' %> + <span class='tooltip tooltip-bottom' data-tip="You're an admin, Harry!" aria-label="Admin"> + <%= icon 'star', aria: { hidden: true } %>app/queries/hexagon_query.rb (4)
55-59: Avoid multiple ST_HexagonGrid calls.Call once and project fields to reduce work.
- hex_grid AS ( - SELECT - (ST_HexagonGrid(#{hex_size}, bbox_utm.geom_utm)).geom as hex_geom_utm, - (ST_HexagonGrid(#{hex_size}, bbox_utm.geom_utm)).i as hex_i, - (ST_HexagonGrid(#{hex_size}, bbox_utm.geom_utm)).j as hex_j - FROM bbox_utm - ), + hex_grid AS ( + SELECT h.geom as hex_geom_utm, h.i as hex_i, h.j as hex_j + FROM bbox_utm, LATERAL ST_HexagonGrid(#{hex_size}, bbox_utm.geom_utm) AS h + ),
81-89: Stabilize ordering and id.Tie-break on hex coordinates to keep deterministic ids.
- row_number() OVER (ORDER BY point_count DESC) as id + row_number() OVER (ORDER BY point_count DESC, hex_i, hex_j) as id @@ - ORDER BY point_count DESC + ORDER BY point_count DESC, hex_i, hex_j
5-5: Cap size: sanity check MAX_HEXAGONS_PER_REQUEST.5000 can be heavy; consider lowering or paging.
9-9: Long parameter list.Consider an options hash or a value object.
spec/services/users/export_import_integration_spec.rb (3)
8-12: Prefer Tempfile over a fixed path to avoid races and filesystem residue.Using a stable filename in tmp is prone to collisions under parallel tests and can leave residue on failures.
Apply:
- let(:temp_archive_path) { Rails.root.join('tmp/test_export.zip') } + let(:temp_archive) { Tempfile.new(['test_export', '.zip']) } @@ - after do - File.delete(temp_archive_path) if File.exist?(temp_archive_path) - end + after do + temp_archive.close! + end @@ - File.open(temp_archive_path, 'wb') do |file| - export_record.file.download { |chunk| file.write(chunk) } - end + temp_archive.binmode + export_record.file.download { |chunk| temp_archive.write(chunk) } + temp_archive.flush @@ - expect(File.exist?(temp_archive_path)).to be true + expect(File.exist?(temp_archive.path)).to be true @@ - import_stats = Users::ImportData.new(target_user, temp_archive_path).import + import_stats = Users::ImportData.new(target_user, temp_archive.path).importAlso applies to: 26-31, 30-30, 38-39
182-188: Style nit: Avoid backslash line continuations in expectations.Use parentheses for readability and to keep RuboCop happy.
-expect(import_stats[:places_created]).to \ - eq(original_places_count), - "Expected #{original_places_count} places to be created, got #{import_stats[:places_created]}" +expect( + import_stats[:places_created] +).to eq( + original_places_count +), "Expected #{original_places_count} places to be created, got #{import_stats[:places_created]}" @@ -expect(imported_places_count).to \ - eq(original_places_count), - "Expected user to have access to #{original_places_count} places, got #{imported_places_count}" +expect( + imported_places_count +).to eq( + original_places_count +), "Expected user to have access to #{original_places_count} places, got #{imported_places_count}"Also applies to: 193-199
34-41: Avoid mutating Rails.logger.level in specs; stub the logger per-exampleMutating Rails.logger.level in spec/services/users/export_import_integration_spec.rb (lines ~34–41) changes global state and can leak across examples. Repo search found no RSpec parallelization config (no config.parallelize), but CI or external parallel runners could still run tests concurrently.
- Replace the assignment with a per-example stub:
allow(Rails.logger).to receive(:level).and_return(Logger::DEBUG)— or better, stub the specific logging methods you rely on (e.g.allow(Rails.logger).to receive(:debug)).spec/requests/exports_spec.rb (2)
57-60: Restore the ActiveJob adapter after the example to avoid cross-test leakage.- ActiveJob::Base.queue_adapter = :test - - expect { post exports_url, params: }.to have_enqueued_job(ExportJob) + old_adapter = ActiveJob::Base.queue_adapter + ActiveJob::Base.queue_adapter = :test + expect { post exports_url, params: }.to have_enqueued_job(ExportJob) + ensure + ActiveJob::Base.queue_adapter = old_adapter
41-45: Unused fixture: points are never realized.
let(:points)is lazy; since nothing references it, these records aren’t created.- let(:points) do + let!(:points) do (1..10).map do |i| create(:point, user:, timestamp: 1.day.ago + i.minutes) end endOr remove entirely if not needed for assertions.
spec/requests/trips_spec.rb (1)
15-21: Align invalid attribute keys with the model to make the failure case explicit.Use
started_at/ended_atlike the valid case; currentstart_date/end_datemay mask validation intent.- let(:invalid_attributes) do - { - name: '', # name can't be blank - start_date: nil, # dates are required - end_date: Date.yesterday # end date can't be before start date - } - end + let(:invalid_attributes) do + { + name: '', # name can't be blank + started_at: nil, # dates are required + ended_at: Date.yesterday # end cannot be before start + } + endapp/controllers/api/v1/settings_controller.rb (1)
14-18: Optional: Assign settings in bulk to reduce per-key writes.- settings_params.each { |key, value| current_api_user.settings[key] = value } + current_api_user.settings = current_api_user.settings.merge(settings_params.to_h)app/views/stats/month.html.erb (1)
1-1: Localize and harden the title.Use I18n and a real Date for the month name; pair with controller validation to avoid invalid dates.
-<% content_for :title, "#{Date::MONTHNAMES[@month]} #{@year} Monthly Digest" %> +<% content_for :title, t('stats.month.title', month: I18n.l(Date.new(@year, @month), format: '%B'), year: @year) %>Add translation:
# config/locales/en.yml en: stats: month: title: "%{month} %{year} Monthly Digest"app/controllers/stats_controller.rb (1)
19-25: Trim ABC complexity by extracting a helper.Move data-loading into a small private method; keeps the action slim and satisfies RuboCop.
Example:
def month @year, @month = parse_year_month!(params) load_month_context(@year, @month) end private def parse_year_month!(params) y = Integer(params[:year], exception: false) m = Integer(params[:month], exception: false) return head :bad_request unless y && m && (1..12).cover?(m) [y, m] end def load_month_context(year, month) @stat = current_user.stats.find_by(year:, month:) @previous_stat = current_user.stats.find_by(year:, month: month - 1) if month > 1 avg_m = current_user.stats.where(year:).average(:distance) @average_distance_this_year = avg_m.to_f / 1000.0 endspec/requests/imports_spec.rb (1)
66-66: Remove trailing whitespace flagged by RuboCop.Tiny cleanup to keep CI green.
- +Also applies to: 103-103
app/controllers/api/v1/visits_controller.rb (1)
95-97: Don’t permit fields you intentionally ignore in updates.You permit latitude/longitude but explicitly skip updating them. Either support updates or drop them from permitted params to avoid API confusion.
-params.require(:visit).permit(:name, :place_id, :status, :latitude, :longitude, :started_at, :ended_at) +params.require(:visit).permit(:name, :place_id, :status, :started_at, :ended_at)Also applies to: 107-113
app/controllers/imports_controller.rb (1)
100-113: Rescue specific errors from find_signed and attachments.Narrow the rescue to known failure modes; handle invalid signatures and missing blobs explicitly.
# Example refinement around find_signed blob = ActiveStorage::Blob.find_signed(signed_id) # ... rescue ActiveSupport::MessageVerifier::InvalidSignature, ActiveRecord::RecordNotFound => e Rails.logger.warn("Invalid or missing blob for signed_id: #{signed_id.class.name}") nilapp/views/stats/index.html.erb (1)
93-93: Axis title says 'Days' but data is by month.Rename x-axis to “Months” for accuracy.
-xtitle: 'Days', +xtitle: 'Months',app/controllers/settings/users_controller.rb (1)
92-95: Zip validation: looks good; consider allowing vendor-specific zip MIME variants.Some browsers report
application/octet-stream. The extension check covers this; no change needed.spec/models/stat_spec.rb (1)
106-264: Prefer AR min/max over raw SQL and add supporting indexes.Reduce adapter-specific SQL and leverage AR for bounds; consider DB indexes on points(user_id, timestamp) for this hot path.
Example (model method sketch):
scope = user.points.where(timestamp: start_date.to_i..end_date.to_i) return nil if scope.empty? { min_lat: scope.minimum(:latitude).to_f, max_lat: scope.maximum(:latitude).to_f, min_lng: scope.minimum(:longitude).to_f, max_lng: scope.maximum(:longitude).to_f, point_count: scope.count }Migration sketch:
add_index :points, [:user_id, :timestamp]app/views/shared/_sharing_modal.html.erb (2)
12-14: Expose CSRF-safe endpoint and method details via data attributes (optional).If the Stimulus controller performs PATCH with JSON, include the CSRF token and HTTP method hints as data-attributes to avoid brittle selector lookups in JS.
Example:
- <div data-controller="sharing-modal" - data-sharing-modal-url-value="<%= sharing_stats_path(year: @year, month: @month) %>"> + <div data-controller="sharing-modal" + data-sharing-modal-url-value="<%= sharing_stats_path(year: @year, month: @month) %>" + data-sharing-modal-csrf-token-value="<%= form_authenticity_token %>" + data-sharing-modal-method-value="PATCH">
20-23: Avoid inline styles; rely on classes for theming.Inline color via style= can complicate CSP and theming. Prefer class composition or CSS variables set by month_color(stat).
- <div class="text-2xl font-semibold text-base-content" style="color: <%= month_color(stat) %>;"> + <div class="text-2xl font-semibold text-base-content" style="--month-color: <%= month_color(stat) %>;"> + <style> .text-2xl[style*="--month-color"] { color: var(--month-color); } </style>spec/queries/hexagon_query_spec.rb (2)
73-83: Clean trailing whitespace and add final newline to satisfy RuboCop.Multiple Layout/TrailingWhitespace offenses and a missing final newline.
- create(:point, - user:, - latitude: 40.7, + create(:point, + user:, + latitude: 40.7, longitude: -74.0, timestamp: Time.new(2024, 6, 15, 12, 0).to_i)Also ensure the file ends with a single newline.
Also applies to: 89-99, 109-121, 140-151, 157-161, 175-186, 187-193, 245-245
51-61: Sanity checks look good; consider asserting geometry validity (optional).You already parse geojson; optionally assert type == 'Polygon' (or whatever HexagonQuery returns) to catch regressions in SQL.
expect(%w[Polygon MultiPolygon]).to include(geojson['type'])Also applies to: 85-104, 118-121, 124-162, 164-173, 183-193, 196-244
app/views/stats/_stat.html.erb (2)
1-2: Use a named route helper instead of a relative string path.String "#{stat.year}/#{stat.month}" is brittle. Prefer an explicit helper to the month view.
-<%= link_to "#{stat.year}/#{stat.month}", +<%= link_to stats_month_path(year: stat.year, month: stat.month), class: "group block p-6 bg-base-100 hover:bg-base-200/50 rounded-xl border border-base-300 hover:border-primary/40 hover:shadow-lg transition-all duration-200 hover:scale-[1.02]" do %>
6-8: Avoid html_safe; compose safe output.Safer to use safe_join with icon helper instead of marking an interpolated string html_safe.
- <%= "#{icon month_icon(stat)} #{Date::MONTHNAMES[stat.month]} #{stat.year}".html_safe %> + <%= safe_join([icon(month_icon(stat)), "#{Date::MONTHNAMES[stat.month]} #{stat.year}"]) %>app/models/stat.rb (1)
84-110: Prefer PostGIS extent for bounds and bind parameters correctly.Using MIN/MAX on lat/lng works but leveraging ST_Extent on lonlat is faster and honors spatial indexes. Also ensure DB binds are passed correctly.
- bounds_result = ActiveRecord::Base.connection.exec_query( - "SELECT MIN(latitude) as min_lat, MAX(latitude) as max_lat, - MIN(longitude) as min_lng, MAX(longitude) as max_lng - FROM points - WHERE user_id = $1 - AND timestamp BETWEEN $2 AND $3", - 'data_bounds_query', - [user.id, start_date.to_i, end_date.to_i] - ).first + sql = <<~SQL + SELECT + ST_YMin(box) AS min_lat, + ST_YMax(box) AS max_lat, + ST_XMin(box) AS min_lng, + ST_XMax(box) AS max_lng + FROM ( + SELECT ST_Extent(lonlat) AS box + FROM points + WHERE user_id = $1 AND timestamp BETWEEN $2 AND $3 + ) AS e + SQL + bounds_result = ActiveRecord::Base.connection.exec_query( + sql, 'data_bounds_query', [[nil, user.id], [nil, start_date.to_i], [nil, end_date.to_i]] + ).firstEnsure points.lonlat exists and PostGIS is enabled. If not, keep current query but still pass binds as [[nil, val], …] for maximum adapter compatibility.
app/services/maps/hexagon_grid.rb (3)
91-131: Split formatting to reduce ABC/length and improve readability.Extract a small helper (e.g., feature_from_row(row)) and a totals reducer.
Apply this diff:
- def format_hexagons(result) - total_points = 0 - - hexagons = result.map do |row| - point_count = row['point_count'].to_i - total_points += point_count - # Parse timestamps and format dates - earliest = row['earliest_point'] ? Time.zone.at(row['earliest_point'].to_f).iso8601 : nil - latest = row['latest_point'] ? Time.zone.at(row['latest_point'].to_f).iso8601 : nil - { - type: 'Feature', - id: row['id'], - geometry: JSON.parse(row['geojson']), - properties: { - hex_id: row['id'], - hex_i: row['hex_i'], - hex_j: row['hex_j'], - hex_size: hex_size, - point_count: point_count, - earliest_point: earliest, - latest_point: latest - } - } - end + def format_hexagons(result) + hexagons = result.map { |row| feature_from_row(row) } + total_points = hexagons.sum { |f| f[:properties][:point_count] } { type: 'FeatureCollection', features: hexagons, metadata: { bbox: [min_lon, min_lat, max_lon, max_lat], area_km2: area_km2.round(2), hex_size_m: hex_size, count: hexagons.count, total_points: total_points, user_id: user_id, date_range: build_date_range_metadata } } end + + def feature_from_row(row) + point_count = row['point_count'].to_i + earliest = row['earliest_point'] ? Time.zone.at(row['earliest_point'].to_f).iso8601 : nil + latest = row['latest_point'] ? Time.zone.at(row['latest_point'].to_f).iso8601 : nil + { + type: 'Feature', + id: row['id'], + geometry: JSON.parse(row['geojson']), + properties: { + hex_id: row['id'], + hex_i: row['hex_i'], + hex_j: row['hex_j'], + hex_size: hex_size, + point_count: point_count, + earliest_point: earliest, + latest_point: latest + } + } + end
147-152: Remove unused viewport helpers/attrs or wire them up.viewport_width/height and viewport_valid? are unused; drop to reduce surface, or hook into validations.
Apply this diff to remove dead code:
- attr_reader :min_lon, :min_lat, :max_lon, :max_lat, :hex_size, :user_id, :start_date, :end_date, :viewport_width, - :viewport_height + attr_reader :min_lon, :min_lat, :max_lon, :max_lat, :hex_size, :user_id, :start_date, :end_date @@ - @viewport_width = params[:viewport_width]&.to_f - @viewport_height = params[:viewport_height]&.to_f @@ - def viewport_valid? - viewport_width && - viewport_height && - viewport_width.positive? && - viewport_height.positive? - end + # (removed viewport helpers)
75-89: Set a short DB statement timeout for heavy spatial queries.Avoid tying up a connection on large bbox; wrap the query in SET LOCAL statement_timeout.
Apply this diff:
def generate_hexagons - query = HexagonQuery.new( + ActiveRecord::Base.connection.execute('SET LOCAL statement_timeout = 3000') # 3s + query = HexagonQuery.new( min_lon:, min_lat:, max_lon:, max_lat:, hex_size:, user_id:, start_date:, end_date: )app/controllers/shared/stats_controller.rb (2)
29-30: Return JSON on not-found for consistency.The endpoint otherwise returns JSON; prefer a JSON 404 body.
Apply this diff:
- return head :not_found unless @stat + unless @stat + return render json: { success: false, message: 'Stat not found' }, status: :not_found + end
24-27: Background long-running toggles.If enabling sharing triggers downstream recomputations (e.g., UUID generation, bounds calc), move to a Sidekiq job per guidelines.
Would you like a minimal Sidekiq worker + controller handoff?
spec/requests/api/v1/maps/hexagons_spec.rb (2)
26-79: Add negative tests for invalid inputs.Cover invalid date format and out-of-range bbox to assert 400.
Apply this diff:
it 'uses custom hex_size when provided' do custom_params = valid_params.merge(hex_size: 500) get '/api/v1/maps/hexagons', params: custom_params, headers: headers expect(response).to have_http_status(:success) end + + it 'rejects invalid date format' do + bad = valid_params.merge(start_date: 'not-a-date') + get '/api/v1/maps/hexagons', params: bad, headers: headers + expect(response).to have_http_status(:bad_request) + end + + it 'rejects out-of-range coordinates' do + bad = valid_params.merge(min_lon: -200, max_lat: 95) + get '/api/v1/maps/hexagons', params: bad, headers: headers + expect(response).to have_http_status(:bad_request) + end
181-223: Bounds endpoint: also test UUID invalid/expired/disabled cases.Mirror index specs for bounds to ensure parity.
Apply this diff:
context 'with public sharing UUID' do let(:stat) { create(:stat, :with_sharing_enabled, user:, year: 2024, month: 6) } @@ end + + it 'returns not found for invalid UUID' do + get '/api/v1/maps/hexagons/bounds', params: { uuid: 'invalid-uuid' } + expect(response).to have_http_status(:not_found) + endapp/views/stats/public_month.html.erb (2)
177-178: Add rel noopener to external link.Prevents tab-napping.
Apply this diff:
- Powered by <a href="https://dawarich.app" class="link link-primary" target="_blank">Dawarich</a>, your personal memories mapper. + Powered by <a href="https://dawarich.app" class="link link-primary" target="_blank" rel="noopener noreferrer">Dawarich</a>, your personal memories mapper.
23-26: Pin and integrity-check third-party scripts.Leaflet has SRI; Protomaps script lacks integrity/defer. Prefer self-host or add defer and explicit version pin.
Apply this diff:
- <script src="https://unpkg.com/[email protected]/dist/protomaps-leaflet.js"></script> + <script src="https://unpkg.com/[email protected]/dist/protomaps-leaflet.js" defer></script>config/routes.rb (1)
79-82: Add UUID format constraint; keep single route if duplication not required.Prevents route hits with obviously invalid UUIDs and reduces ambiguity.
Apply this diff:
-# Public shared stats routes -get 'shared/stats/:uuid', to: 'shared/stats#show', as: :shared_stat -# Backward compatibility -get 'shared/stats/:uuid', to: 'shared/stats#show', as: :public_stat +# Public shared stats routes +get 'shared/stats/:uuid', to: 'shared/stats#show', as: :shared_stat, constraints: { uuid: /\A[0-9a-fA-F-]{36}\z/ } +# Backward compatibility (duplicate path intentionally for alias) +get 'shared/stats/:uuid', to: 'shared/stats#show', as: :public_stat, constraints: { uuid: /\A[0-9a-fA-F-]{36}\z/ }app/views/stats/_month.html.erb (2)
24-24: Stray tilde in UI.Remove the leading
~before distance output.- <div class="stat-value text-success">~<%= distance_traveled(current_user, stat) %></div> + <div class="stat-value text-success"><%= distance_traveled(current_user, stat) %></div>
48-49: Typo in helper name.Rename
x_than_prevopis_countries_visited→x_than_previous_countries_visitedfor clarity; update helper method accordingly (see StatsHelper comment).- <%= x_than_prevopis_countries_visited(stat, previous_stat) %> + <%= x_than_previous_countries_visited(stat, previous_stat) %>app/controllers/api/v1/maps/hexagons_controller.rb (1)
59-69: Permit consistency: lon vs lng.You permit
min_lon/max_lonbut returnmin_lng/max_lng. Ensure frontends expect the names you return (they do in public controller). If not, align terminology or document.app/javascript/controllers/stat_page_controller.js (1)
105-111: Handle large months (pagination).
per_page=1000can truncate data. Consider iterating pages until exhausted or adding a dedicated month-points endpoint returning all (with server-side limits).app/helpers/stats_helper.rb (2)
53-66: Rename helper to fix typo and improve readability.-def x_than_prevopis_countries_visited(stat, previous_stat) +def x_than_previous_countries_visited(stat, previous_stat) return '' unless previous_stat previous_countries = previous_stat.toponyms.count { _1['country'] } current_countries = stat.toponyms.count { _1['country'] } difference = current_countries - previous_countries return 'Same as previous month' if difference.zero? more_or_less = difference.positive? ? 'more' : 'less' countries_word = pluralize(difference.abs, 'country') "#{countries_word} #{more_or_less} than previous month" endRemember to update the call site in the view.
122-129: Prefer a lookup table over a wide case for icons.Not urgent, but a small map keeps RuboCop happy and is easier to extend.
- def month_icon(stat) - case stat.month - when 1..2, 12 then 'snowflake' - when 3..5 then 'flower' - when 6..8 then 'tree-palm' - when 9..11 then 'leaf' - end - end + def month_icon(stat) + ICON_BY_MONTH ||= { + winter: 'snowflake', spring: 'flower', summer: 'tree-palm', fall: 'leaf' + }.freeze + ICON_BY_MONTH[ + case stat.month + when 1..2, 12 then :winter + when 3..5 then :spring + when 6..8 then :summer + else :fall + end + ] + endapp/javascript/maps/hexagon_grid.js (4)
175-176: Remove confusing debug log.This “should not happen” log will confuse and spam consoles. Drop or gate behind a debug flag.
- console.log('❌ Using ORIGINAL loadHexagons method (should not happen for public sharing)'); + // console.debug('[HexagonGrid] loadHexagons()');
212-214: Don’t set Content-Type on GET; prefer Accept.Setting Content-Type on GET is unnecessary and sometimes harmful. Use Accept instead.
- headers: { - 'Content-Type': 'application/json' - } + headers: { 'Accept': 'application/json' }
133-140: Bounds change heuristic and anti-meridian edge case.Your area/intersection math assumes lon increases left→right and doesn’t handle anti-meridian crossings. This can cause unnecessary reloads near ±180° and odd zoom/pan heuristics.
If your user base can cross the dateline, consider normalizing longitudes to a common range (e.g., [-180, 180]) and special-casing wrap-around intersections. I can draft a patch if needed. Verify with pans across 179°E↔179°W.
Also applies to: 145-169
317-326: updateStyle overwrites data-driven styling.Calling setStyle with a flat object erases the density-based styling. Either reapply via a style function using the last computed maxPoints, or document that updateStyle opts-out of data-driven colors.
I can wire this to reuse styleHexagonByData with a stored this._lastMaxPoints if you want.
app/javascript/controllers/public_stat_map_controller.js (5)
239-242: Remove unused maxPoints calculation.You compute maxPoints but don’t use it in static styling.
- // Calculate max point count for color scaling - const maxPoints = Math.max(...geojsonData.features.map(f => f.properties.point_count)); - const staticHexagonLayer = L.geoJSON(geojsonData, {
176-180: UTC-safe month boundaries (avoid TZ drift).Constructing dates with local midnight then calling toISOString can shift day boundaries in UTC. Consider using Date.UTC to pin the month range in UTC, if that’s what the API expects.
- const startDate = new Date(this.yearValue, this.monthValue - 1, 1); - const endDate = new Date(this.yearValue, this.monthValue, 0, 23, 59, 59); + const startDate = new Date(Date.UTC(this.yearValue, this.monthValue - 1, 1, 0, 0, 0)); + const endDate = new Date(Date.UTC(this.yearValue, this.monthValue, 0, 23, 59, 59));Confirm how the backend parses start_date/end_date (UTC vs local) before merging.
6-14: Trim or gate verbose console logs.There are several console.log calls with emojis useful for debugging but noisy in prod. Consider gating behind a debug flag or removing.
I can add a tiny logger utility (no-op in production) if you want.
Also applies to: 16-22
7-14: Target “container” is defined but unused.You declare static targets = ["container"] but instantiate the map on this.element. Either use this.containerTarget for clarity or drop the target.
I can prep a small diff either way.
Also applies to: 33-43
285-300: Improve hover UX: bring features to front.Raise hovered hexagon above neighbors for clearer highlight.
onHexagonMouseOver(e) { const layer = e.target; // Store original style before changing if (!layer._originalStyle) { layer._originalStyle = { fillOpacity: layer.options.fillOpacity, weight: layer.options.weight, opacity: layer.options.opacity }; } layer.setStyle({ fillOpacity: 0.8, weight: 2, opacity: 1.0 }); + if (layer.bringToFront) layer.bringToFront(); }
| .loading-spinner::before { | ||
| content: '🔵'; | ||
| content: ''; | ||
| font-size: 18px; | ||
| animation: spinner 1s linear infinite; | ||
| } |
There was a problem hiding this comment.
Spinner becomes invisible after removing the glyph
With empty content, nothing is rendered; the animation runs on an invisible pseudo-element. Convert it to a CSS border spinner.
Apply:
.loading-spinner::before {
- content: '';
- font-size: 18px;
- animation: spinner 1s linear infinite;
+ content: '';
+ display: inline-block;
+ width: 1em;
+ height: 1em;
+ border: 2px solid #c0c0c0;
+ border-top-color: #397bb5;
+ border-radius: 50%;
+ animation: spinner 1s linear infinite;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .loading-spinner::before { | |
| content: '🔵'; | |
| content: ''; | |
| font-size: 18px; | |
| animation: spinner 1s linear infinite; | |
| } | |
| .loading-spinner::before { | |
| content: ''; | |
| display: inline-block; | |
| width: 1em; | |
| height: 1em; | |
| border: 2px solid #c0c0c0; | |
| border-top-color: #397bb5; | |
| border-radius: 50%; | |
| animation: spinner 1s linear infinite; | |
| } |
🤖 Prompt for AI Agents
In app/assets/stylesheets/application.css around lines 94 to 98, the
.loading-spinner::before pseudo-element has empty content and only a
font-size/animation, so it’s invisible; replace the pseudo-element’s styles to
create a visible CSS border spinner: keep content: '' and animation: spinner 1s
linear infinite, add explicit width and height (e.g. 18px), display:
inline-block or block, border: 2px solid currentColor (or a chosen color),
border-top-color: transparent (or border-color variations) and border-radius:
50% so the animated rotation visually appears; ensure any existing spinner
keyframes remain unchanged.
| points_relation = @target_user.points.where(timestamp: @start_date..@end_date) | ||
| point_count = points_relation.count | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Index requirement for performance.
Bounds and point counts over ranges need points(user_id, timestamp) composite index. Please ensure it exists.
Run to confirm:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP 'create_table\s+"points"|add_index.+points' db/schema.rb -C2 || true
rg -nP 'add_index\(.+points.+(user_id.*timestamp|timestamp.*user_id)' db/migrate -n -C2 || trueLength of output: 125
Index requirement for performance — add composite index on points(user_id, timestamp).
app/controllers/api/v1/maps/hexagons_controller.rb (lines 28–30) does range/counts on @target_user.points; db/schema.rb shows the points table but no composite index and no matching add_index in db/migrate. Add a migration:
add_index :points, [:user_id, :timestamp]
Run the migration and verify query performance.
🤖 Prompt for AI Agents
In app/controllers/api/v1/maps/hexagons_controller.rb around lines 28–30, the
controller performs a range/count on @target_user.points which will be slow
without a composite index; add a new Rails migration that creates a composite
index on the points table for [:user_id, :timestamp] (e.g. add_index :points,
[:user_id, :timestamp]), run rails db:migrate, and then verify the query plan
and performance to confirm the index is used.
| if point_count.positive? | ||
| bounds_result = ActiveRecord::Base.connection.exec_query( | ||
| "SELECT MIN(latitude) as min_lat, MAX(latitude) as max_lat, | ||
| MIN(longitude) as min_lng, MAX(longitude) as max_lng | ||
| FROM points | ||
| WHERE user_id = $1 | ||
| AND timestamp BETWEEN $2 AND $3", | ||
| 'bounds_query', | ||
| [@target_user.id, @start_date.to_i, @end_date.to_i] | ||
| ).first | ||
|
|
There was a problem hiding this comment.
Replace raw SQL with AR aggregations; fix potential timestamp type mismatch.
exec_query with $1-style binds and to_i assumes integer timestamps and Postgres-specific binding. Prefer AR aggregations to avoid SQL dialect and quoting issues.
- if point_count.positive?
- bounds_result = ActiveRecord::Base.connection.exec_query(
- "SELECT MIN(latitude) as min_lat, MAX(latitude) as max_lat,
- MIN(longitude) as min_lng, MAX(longitude) as max_lng
- FROM points
- WHERE user_id = $1
- AND timestamp BETWEEN $2 AND $3",
- 'bounds_query',
- [@target_user.id, @start_date.to_i, @end_date.to_i]
- ).first
+ if point_count.positive?
+ min_lat, max_lat, min_lng, max_lng =
+ points_relation.pick(
+ Arel.sql('MIN(latitude)'),
+ Arel.sql('MAX(latitude)'),
+ Arel.sql('MIN(longitude)'),
+ Arel.sql('MAX(longitude)')
+ )- render json: {
- min_lat: bounds_result['min_lat'].to_f,
- max_lat: bounds_result['max_lat'].to_f,
- min_lng: bounds_result['min_lng'].to_f,
- max_lng: bounds_result['max_lng'].to_f,
- point_count: point_count
- }
+ render json: {
+ min_lat: min_lat.to_f,
+ max_lat: max_lat.to_f,
+ min_lng: min_lng.to_f,
+ max_lng: max_lng.to_f,
+ point_count: point_count
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if point_count.positive? | |
| bounds_result = ActiveRecord::Base.connection.exec_query( | |
| "SELECT MIN(latitude) as min_lat, MAX(latitude) as max_lat, | |
| MIN(longitude) as min_lng, MAX(longitude) as max_lng | |
| FROM points | |
| WHERE user_id = $1 | |
| AND timestamp BETWEEN $2 AND $3", | |
| 'bounds_query', | |
| [@target_user.id, @start_date.to_i, @end_date.to_i] | |
| ).first | |
| if point_count.positive? | |
| min_lat, max_lat, min_lng, max_lng = | |
| points_relation.pick( | |
| Arel.sql('MIN(latitude)'), | |
| Arel.sql('MAX(latitude)'), | |
| Arel.sql('MIN(longitude)'), | |
| Arel.sql('MAX(longitude)') | |
| ) | |
| render json: { | |
| min_lat: min_lat.to_f, | |
| max_lat: max_lat.to_f, | |
| min_lng: min_lng.to_f, | |
| max_lng: max_lng.to_f, | |
| point_count: point_count | |
| } |
🤖 Prompt for AI Agents
In app/controllers/api/v1/maps/hexagons_controller.rb around lines 31 to 41,
replace the raw exec_query SQL with ActiveRecord aggregation calls and correct
timestamp types: use Point.where(user_id: @target_user.id, timestamp:
@start_date..@end_date) (or the model name in your app) and call
minimum(:latitude), maximum(:latitude), minimum(:longitude), maximum(:longitude)
(or a single select with ARel) to get bounds; do not convert dates to
integers—pass Ruby Time/DateTime/Range so AR/PG adapter handles types and
bindings safely.
| def set_authenticated_context | ||
| @target_user = current_api_user | ||
| @start_date = params[:start_date] | ||
| @end_date = params[:end_date] | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parse and validate date params; accept start_at/end_at; normalize to Time.zone.
Current code forwards raw strings; can break downstream and in comparisons.
def set_authenticated_context
@target_user = current_api_user
- @start_date = params[:start_date]
- @end_date = params[:end_date]
+ @start_date = parse_time_param(params[:start_date] || params[:start_at])
+ @end_date = parse_time_param(params[:end_date] || params[:end_at])
+ unless @start_date && @end_date
+ render json: { error: 'Invalid or missing date range' }, status: :bad_request and return
+ end
end
+
+def parse_time_param(val)
+ return nil if val.blank?
+ Time.zone.parse(val)
+rescue ArgumentError
+ nil
+end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def set_authenticated_context | |
| @target_user = current_api_user | |
| @start_date = params[:start_date] | |
| @end_date = params[:end_date] | |
| end | |
| def set_authenticated_context | |
| @target_user = current_api_user | |
| @start_date = parse_time_param(params[:start_date] || params[:start_at]) | |
| @end_date = parse_time_param(params[:end_date] || params[:end_at]) | |
| unless @start_date && @end_date | |
| render json: { error: 'Invalid or missing date range' }, status: :bad_request and return | |
| end | |
| end | |
| def parse_time_param(val) | |
| return nil if val.blank? | |
| Time.zone.parse(val) | |
| rescue ArgumentError | |
| nil | |
| end |
🤖 Prompt for AI Agents
In app/controllers/api/v1/maps/hexagons_controller.rb around lines 91-95, the
controller currently assigns raw param strings to @start_date/@end_date; update
it to accept params[:start_date] or params[:start_at] (and params[:end_date] or
params[:end_at]), parse them with Time.zone.parse so values are normalized to
the app timezone, validate parsing (treat blank/missing as nil), and rescue
invalid parse errors to return a 400 with a clear message; set @start_date and
@end_date to Time objects (or nil) for downstream comparisons.
| gem 'sidekiq-cron' | ||
| gem 'sidekiq-limit_fetch' | ||
| gem 'sprockets-rails' | ||
| gem 'stackprof' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Scope stackprof to dev/test and avoid auto-require.
Prevents unnecessary require in production.
-gem 'stackprof'
+group :development, :test do
+ gem 'stackprof', require: false
+end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gem 'stackprof' | |
| group :development, :test do | |
| gem 'stackprof', require: false | |
| end |
🤖 Prompt for AI Agents
In Gemfile around line 48, the gem 'stackprof' is added without environment
scoping and is auto-required; change it to the development and test groups and
disable auto-require so it isn't loaded in production. Replace the single gem
line with a gem declaration scoped to group :development, :test and add require:
false (so Bundler doesn't auto-require it), then run bundle install to update
the lockfile.
| return unless original_office_points && target_office_points | ||
|
|
||
| expect(target_office_points.import.name).to eq(original_office_points.import.name) if original_office_points.import | ||
| if original_office_points.country | ||
| expect(target_office_points.country.name).to eq(original_office_points.country.name) | ||
| end | ||
| expect(target_office_points.visit.name).to eq(original_office_points.visit.name) if original_office_points.visit | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t silently skip core relationship assertions. Fail fast if expected anchor points are missing.
The early return can mask regressions in import/export logic.
- return unless original_office_points && target_office_points
-
- expect(target_office_points.import.name).to eq(original_office_points.import.name) if original_office_points.import
+ expect(original_office_points).to be_present
+ expect(target_office_points).to be_present
+ expect(target_office_points.import.name).to eq(original_office_points.import.name) if original_office_points.import
if original_office_points.country
expect(target_office_points.country.name).to eq(original_office_points.country.name)
end
expect(target_office_points.visit.name).to eq(original_office_points.visit.name) if original_office_points.visit📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return unless original_office_points && target_office_points | |
| expect(target_office_points.import.name).to eq(original_office_points.import.name) if original_office_points.import | |
| if original_office_points.country | |
| expect(target_office_points.country.name).to eq(original_office_points.country.name) | |
| end | |
| expect(target_office_points.visit.name).to eq(original_office_points.visit.name) if original_office_points.visit | |
| end | |
| expect(original_office_points).to be_present | |
| expect(target_office_points).to be_present | |
| expect(target_office_points.import.name).to eq(original_office_points.import.name) if original_office_points.import | |
| if original_office_points.country | |
| expect(target_office_points.country.name).to eq(original_office_points.country.name) | |
| end | |
| expect(target_office_points.visit.name).to eq(original_office_points.visit.name) if original_office_points.visit | |
| end |
🧰 Tools
🪛 RuboCop (1.76.1)
[convention] 316-347: Assignment Branch Condition size for verify_relationships_preserved is too high. [<10, 74, 5> 74.84/17]
(Metrics/AbcSize)
[convention] 316-347: Method has too many lines. [24/10]
(Metrics/MethodLength)
🤖 Prompt for AI Agents
In spec/services/users/export_import_integration_spec.rb around lines 340 to
347, remove the early "return unless original_office_points &&
target_office_points" and instead add explicit failing assertions that both
original_office_points and target_office_points are present; then replace the
conditional relationship checks with explicit presence expectations (e.g.
expect(original_office_points.import).to be_present) followed by the equality
assertions comparing .name for import, country and visit so the spec fails fast
when anchor points or relationships are missing rather than silently skipping
assertions.
| return unless original_export&.file&.attached? | ||
|
|
||
| expect(target_export).to be_present | ||
| expect(target_export.file).to be_attached | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert source export presence instead of early return; otherwise you can hide missing file regressions.
- return unless original_export&.file&.attached?
-
- expect(target_export).to be_present
- expect(target_export.file).to be_attached
+ expect(original_export&.file).to be_attached
+ expect(target_export).to be_present
+ expect(target_export.file).to be_attached📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return unless original_export&.file&.attached? | |
| expect(target_export).to be_present | |
| expect(target_export.file).to be_attached | |
| end | |
| expect(original_export&.file).to be_attached | |
| expect(target_export).to be_present | |
| expect(target_export.file).to be_attached | |
| end |
🧰 Tools
🪛 RuboCop (1.76.1)
[convention] 356-379: Assignment Branch Condition size for verify_files_restored is too high. [<7, 52, 10> 53.41/17]
(Metrics/AbcSize)
[convention] 356-379: Cyclomatic complexity for verify_files_restored is too high. [10/7]
(Metrics/CyclomaticComplexity)
[convention] 356-379: Method has too many lines. [16/10]
(Metrics/MethodLength)
[convention] 356-379: Perceived complexity for verify_files_restored is too high. [10/8]
(Metrics/PerceivedComplexity)
🤖 Prompt for AI Agents
In spec/services/users/export_import_integration_spec.rb around lines 375-379,
the test currently uses an early return when original_export or its file isn’t
attached, which can silently hide regressions; replace the guard with explicit
expectations asserting that original_export is present and its file is attached
(e.g. expect(original_export).to be_present and expect(original_export.file).to
be_attached) so the spec fails loudly when the source export or its file is
missing, then keep the existing assertions for target_export.
| // Disable all dynamic behavior by removing event listeners | ||
| this.map.off('moveend'); | ||
| this.map.off('zoomend'); | ||
|
|
There was a problem hiding this comment.
Don’t remove all map listeners.
map.off('moveend') / map.off('zoomend') without a handler removes unrelated listeners (including ones from other layers/controllers). Not needed since hexagonGrid.hide() already short-circuits its handlers.
Apply:
- // Disable all dynamic behavior by removing event listeners
- this.map.off('moveend');
- this.map.off('zoomend');
+ // Dynamic behavior is disabled by hexagonGrid.hide(); no need to remove global listeners.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Disable all dynamic behavior by removing event listeners | |
| this.map.off('moveend'); | |
| this.map.off('zoomend'); | |
| // Dynamic behavior is disabled by hexagonGrid.hide(); no need to remove global listeners. |
🤖 Prompt for AI Agents
In app/javascript/controllers/public_stat_map_controller.js around lines 121 to
124, the calls this.map.off('moveend') and this.map.off('zoomend') remove all
handlers for those events (including handlers from other layers/controllers);
since hexagonGrid.hide() already short-circuits its handlers, remove these two
lines entirely. If you must unregister only the controller's own handlers
instead, call this.map.off('moveend',
<reference-to-this-controller-moveend-handler>) and this.map.off('zoomend',
<reference-to-this-controller-zoomend-handler>) so you only remove the specific
functions you attached.
| fetch(url, { | ||
| method: 'PATCH', | ||
| headers: { | ||
| 'X-CSRF-Token': document.querySelector('meta[name="csrf-token"]').content, | ||
| 'X-Requested-With': 'XMLHttpRequest' | ||
| }, | ||
| body: formData | ||
| }) | ||
| .then(response => response.json()) | ||
| .then(data => { | ||
| this.hideLoadingState() | ||
|
|
||
| if (data.success) { | ||
| // Update sharing link if provided | ||
| if (data.sharing_url) { | ||
| this.sharingLinkTarget.value = data.sharing_url | ||
| } | ||
|
|
||
| // Show a subtle notification for auto-save | ||
| this.showNotification("✓ Auto-saved", "success") | ||
| } else { | ||
| this.showNotification("Failed to save settings. Please try again.", "error") | ||
| } | ||
| }) | ||
| .catch(error => { | ||
| console.error('Error:', error) | ||
| this.hideLoadingState() | ||
| this.showNotification("Failed to save settings. Please try again.", "error") | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure cookies are sent; handle non-OK responses.
- fetch(url, {
+ fetch(url, {
method: 'PATCH',
headers: {
'X-CSRF-Token': document.querySelector('meta[name="csrf-token"]').content,
- 'X-Requested-With': 'XMLHttpRequest'
+ 'X-Requested-With': 'XMLHttpRequest',
+ 'Accept': 'application/json'
},
- body: formData
+ body: formData,
+ credentials: 'same-origin'
})
- .then(response => response.json())
+ .then(response => {
+ if (!response.ok) throw new Error(`HTTP ${response.status}`);
+ return response.json();
+ })
.then(data => {
this.hideLoadingState()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fetch(url, { | |
| method: 'PATCH', | |
| headers: { | |
| 'X-CSRF-Token': document.querySelector('meta[name="csrf-token"]').content, | |
| 'X-Requested-With': 'XMLHttpRequest' | |
| }, | |
| body: formData | |
| }) | |
| .then(response => response.json()) | |
| .then(data => { | |
| this.hideLoadingState() | |
| if (data.success) { | |
| // Update sharing link if provided | |
| if (data.sharing_url) { | |
| this.sharingLinkTarget.value = data.sharing_url | |
| } | |
| // Show a subtle notification for auto-save | |
| this.showNotification("✓ Auto-saved", "success") | |
| } else { | |
| this.showNotification("Failed to save settings. Please try again.", "error") | |
| } | |
| }) | |
| .catch(error => { | |
| console.error('Error:', error) | |
| this.hideLoadingState() | |
| this.showNotification("Failed to save settings. Please try again.", "error") | |
| }) | |
| } | |
| fetch(url, { | |
| method: 'PATCH', | |
| headers: { | |
| 'X-CSRF-Token': document.querySelector('meta[name="csrf-token"]').content, | |
| 'X-Requested-With': 'XMLHttpRequest', | |
| 'Accept': 'application/json' | |
| }, | |
| body: formData, | |
| credentials: 'same-origin' | |
| }) | |
| .then(response => { | |
| if (!response.ok) throw new Error(`HTTP ${response.status}`); | |
| return response.json(); | |
| }) | |
| .then(data => { | |
| this.hideLoadingState() | |
| if (data.success) { | |
| // Update sharing link if provided | |
| if (data.sharing_url) { | |
| this.sharingLinkTarget.value = data.sharing_url | |
| } | |
| // Show a subtle notification for auto-save | |
| this.showNotification("✓ Auto-saved", "success") | |
| } else { | |
| this.showNotification("Failed to save settings. Please try again.", "error") | |
| } | |
| }) | |
| .catch(error => { | |
| console.error('Error:', error) | |
| this.hideLoadingState() | |
| this.showNotification("Failed to save settings. Please try again.", "error") | |
| }) | |
| } |
🤖 Prompt for AI Agents
In app/javascript/controllers/sharing_modal_controller.js around lines 47 to 76,
the fetch call does not send cookies and does not handle non-OK HTTP responses
before calling response.json(); update the fetch options to include credentials
(e.g., credentials: 'same-origin' or 'include' as appropriate), then on response
check response.ok and handle non-OK statuses by hiding the loading state and
showing an error notification (and logging the status/text) instead of blindly
calling response.json(); also guard response.json() with try/catch to handle
invalid JSON and ensure hideLoadingState runs on all failure paths.
| init() { | ||
| // Create the hexagon layer group | ||
| this.hexagonLayer = L.layerGroup(); | ||
|
|
||
| // Bind map events | ||
| this.map.on('moveend', this.debounce(this.onMapMove.bind(this), this.options.debounceDelay)); | ||
| this.map.on('zoomend', this.onZoomChange.bind(this)); | ||
|
|
There was a problem hiding this comment.
Don’t nuke all map listeners; bind/unbind exact handlers.
You’re calling map.off('moveend') / map.off('zoomend') without a handler in destroy(), which removes unrelated listeners registered elsewhere. Store your bound handlers and unbind those specifically.
Apply:
init() {
// Create the hexagon layer group
this.hexagonLayer = L.layerGroup();
// Bind map events
- this.map.on('moveend', this.debounce(this.onMapMove.bind(this), this.options.debounceDelay));
- this.map.on('zoomend', this.onZoomChange.bind(this));
+ this._onMoveEnd = this.debounce(this.onMapMove.bind(this), this.options.debounceDelay);
+ this._onZoomEnd = this.onZoomChange.bind(this);
+ this.map.on('moveend', this._onMoveEnd);
+ this.map.on('zoomend', this._onZoomEnd); destroy() {
this.hide();
- this.map.off('moveend');
- this.map.off('zoomend');
+ if (this._onMoveEnd) {
+ this.map.off('moveend', this._onMoveEnd);
+ this._onMoveEnd = null;
+ }
+ if (this._onZoomEnd) {
+ this.map.off('zoomend', this._onZoomEnd);
+ this._onZoomEnd = null;
+ }
this.hexagonLayer = null;
this.lastBounds = null;
}Also applies to: 331-337
🤖 Prompt for AI Agents
In app/javascript/maps/hexagon_grid.js around lines 31-38 (and also apply the
same change to 331-337), you currently register anonymous/bound callbacks
directly with this.map.on which prevents precise unbinding; store the bound
handler references as properties (e.g. this._onMoveEnd =
this.debounce(this.onMapMove.bind(this), this.options.debounceDelay) and
this._onZoomEnd = this.onZoomChange.bind(this)), use those properties when
calling this.map.on, and in destroy() call this.map.off('moveend',
this._onMoveEnd) and this.map.off('zoomend', this._onZoomEnd) so you only remove
the handlers you added. Ensure you clean up the stored handler references (set
to null) after unbinding.
| // Get current date range from URL parameters | ||
| const urlParams = new URLSearchParams(window.location.search); | ||
| const startDate = urlParams.get('start_at'); | ||
| const endDate = urlParams.get('end_at'); | ||
|
|
There was a problem hiding this comment.
URL param mismatch may skip date filtering.
You read start_at/end_at but send start_date/end_date. If pages set start_date/end_date (as elsewhere), dynamic loads may fetch unbounded data.
Apply:
- const startDate = urlParams.get('start_at');
- const endDate = urlParams.get('end_at');
+ const startDate = urlParams.get('start_date') || urlParams.get('start_at');
+ const endDate = urlParams.get('end_date') || urlParams.get('end_at');Also applies to: 206-209
🤖 Prompt for AI Agents
In app/javascript/maps/hexagon_grid.js around lines 187-191 (and similarly lines
206-209), the code reads URL params 'start_at'/'end_at' but elsewhere the app
uses 'start_date'/'end_date', so date filtering can be skipped; update the
parameter reads to use 'start_date' and 'end_date' (or accept both names by
falling back from one to the other) and ensure those variables are used when
building the dynamic fetch/query so the same param names are sent and applied;
make the same change for the later block at 206-209 to keep behavior consistent.
| // Calculate max point count for color scaling | ||
| const maxPoints = Math.max(...geojsonData.features.map(f => f.properties.point_count)); | ||
|
|
||
| const geoJsonLayer = L.geoJSON(geojsonData, { | ||
| style: (feature) => this.styleHexagonByData(feature, maxPoints), | ||
| onEachFeature: (feature, layer) => { |
There was a problem hiding this comment.
Guard against NaN/zero maxPoints and clamp opacities; also use configured color.
If all point_count are 0/undefined, maxPoints becomes NaN/0 → opacity becomes NaN and styling breaks. Also, respect options.style.color instead of hardcoding.
Apply:
- // Calculate max point count for color scaling
- const maxPoints = Math.max(...geojsonData.features.map(f => f.properties.point_count));
+ // Calculate max point count for color scaling (guard against 0/NaN)
+ const rawMax = Math.max(...geojsonData.features.map(f => Number(f.properties.point_count) || 0));
+ const maxPoints = Number.isFinite(rawMax) && rawMax > 0 ? rawMax : 1;
+ this._lastMaxPoints = maxPoints; styleHexagonByData(feature, maxPoints) {
const props = feature.properties;
const pointCount = props.point_count || 0;
- // Calculate opacity based on point density (0.2 to 0.8)
- const opacity = 0.2 + (pointCount / maxPoints) * 0.6;
-
- let color = '#3388ff'
+ const safeMax = Number.isFinite(maxPoints) && maxPoints > 0 ? maxPoints : (this._lastMaxPoints || 1);
+ const ratio = Math.min(1, Math.max(0, pointCount / safeMax));
+ // Calculate opacity based on point density (0.2 to 0.8)
+ const opacity = 0.2 + ratio * 0.6;
+ const color = (this.options.style && this.options.style.color) ? this.options.style.color : '#3388ff';
return {
fillColor: color,
fillOpacity: opacity,
color: color,
weight: 1,
- opacity: opacity + 0.2
+ opacity: Math.min(1, opacity + 0.2)
};
}Also applies to: 281-296
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/jobs/cache/preheating_job.rb (1)
7-31: Consider extracting cache key generation to a centralized locationThe cache keys are duplicated between this job and
Cache::Clean. Consider extracting them to a module or constant to maintain consistency and avoid synchronization issues.Create a cache key helper module:
# app/services/cache/keys.rb module Cache module Keys def self.user_years_tracked(user_id) "dawarich/user_#{user_id}_years_tracked" end def self.user_points_geocoded_stats(user_id) "dawarich/user_#{user_id}_points_geocoded_stats" end def self.user_countries_visited(user_id) "dawarich/user_#{user_id}_countries_visited" end def self.user_cities_visited(user_id) "dawarich/user_#{user_id}_cities_visited" end end endThen update this job to use the centralized keys:
User.find_each do |user| Rails.cache.write( - "dawarich/user_#{user.id}_years_tracked", + Cache::Keys.user_years_tracked(user.id), user.years_tracked, expires_in: 1.day ) Rails.cache.write( - "dawarich/user_#{user.id}_points_geocoded_stats", + Cache::Keys.user_points_geocoded_stats(user.id), StatsQuery.new(user).cached_points_geocoded_stats, expires_in: 1.day ) Rails.cache.write( - "dawarich/user_#{user.id}_countries_visited", + Cache::Keys.user_countries_visited(user.id), user.countries_visited_uncached, expires_in: 1.day ) Rails.cache.write( - "dawarich/user_#{user.id}_cities_visited", + Cache::Keys.user_cities_visited(user.id), user.cities_visited_uncached, expires_in: 1.day ) endspec/jobs/users/mailer_sending_job_spec.rb (1)
110-118: Test expectation doesn't match the descriptionThe test description says "raises ActiveRecord::RecordNotFound" but the expectation is
not_to raise_error. This is inconsistent and confusing. Update the test description to match the new behavior.context 'when user is deleted' do - it 'raises ActiveRecord::RecordNotFound' do + it 'handles missing user gracefully without raising error' do user.destroy expect do described_class.perform_now(user.id, 'welcome') end.not_to raise_error end end
♻️ Duplicate comments (4)
CHANGELOG.md (2)
16-16: Fix grammar in cron job description.Apply this diff to fix the grammar:
-A cron job to generate daily tracks for users with new points since their last track generation. Being run every 4 hours. +A cron job to generate daily tracks for users with new points since their last track generation. Runs every 4 hours.
24-24: Clarify build architecture policy for patch vs minor releases.Apply this diff to be more precise about the build policy:
-Minor versions are now being built only for amd64 architecture to speed up the build process. +Patch versions (x.y.z where z > 0) are now built only for amd64 to speed up the build process; minor versions (x.y.0) continue to build for all supported platforms.app/views/stats/index.html.erb (1)
43-43: Add accessible label to the icon-only refresh link.Screen readers need context for icon-only buttons. Add aria-label and title attributes for better accessibility.
-<%= link_to icon('refresh-ccw'), update_year_month_stats_path(year, :all), data: { turbo_method: :put }, class: 'text-sm text-gray-500 hover:text-primary' %> +<%= link_to icon('refresh-ccw'), + update_year_month_stats_path(year, :all), + data: { turbo_method: :put }, + class: 'text-sm text-gray-500 hover:text-primary', + aria: { label: "Refresh statistics for #{year}" }, + title: "Refresh statistics for #{year}" %>app/helpers/stats_helper.rb (1)
69-77: Fix nil safety and unit consistency issues (duplicate from previous review).The existing review comment highlighting nil safety concerns and unit consistency issues is still valid and should be addressed.
🧹 Nitpick comments (13)
app/jobs/bulk_stats_calculating_job.rb (1)
7-7: Consider deduplicating user IDs to prevent redundant stats calculations.The current implementation concatenates
User.active.pluck(:id)andUser.trial.pluck(:id)without deduplicating, which could result in processing the same user twice if they appear in both scopes. This would cause unnecessary duplicate stats calculations.- user_ids = User.active.pluck(:id) + User.trial.pluck(:id) + user_ids = (User.active.pluck(:id) + User.trial.pluck(:id)).uniqapp/services/cache/clean.rb (2)
10-11: Consider combining cache deletion operations for performanceThe new methods
delete_points_geocoded_stats_cacheanddelete_countries_cities_cacheiterate through all users separately. This could be optimized by combining the cache deletion operations into a single iteration.- delete_points_geocoded_stats_cache - delete_countries_cities_cache + delete_user_cachesAnd combine the methods:
- def delete_points_geocoded_stats_cache - User.find_each do |user| - Rails.cache.delete("dawarich/user_#{user.id}_points_geocoded_stats") - end - end - - def delete_countries_cities_cache - User.find_each do |user| - Rails.cache.delete("dawarich/user_#{user.id}_countries_visited") - Rails.cache.delete("dawarich/user_#{user.id}_cities_visited") - end - end + def delete_user_caches + User.find_each do |user| + Rails.cache.delete_multi([ + "dawarich/user_#{user.id}_points_geocoded_stats", + "dawarich/user_#{user.id}_countries_visited", + "dawarich/user_#{user.id}_cities_visited" + ]) + end + endThis reduces the number of database queries from 3 separate
User.find_eachcalls to just 1, improving performance when there are many users.
25-29: Consider using batch cache operationsThe current implementation makes individual cache delete calls for each user. Consider using
Rails.cache.delete_multiif your cache store supports it for better performance.def delete_years_tracked_cache - User.find_each do |user| - Rails.cache.delete("dawarich/user_#{user.id}_years_tracked") - end + keys = User.pluck(:id).map { |id| "dawarich/user_#{id}_years_tracked" } + Rails.cache.delete_multi(keys) if keys.any? end def delete_points_geocoded_stats_cache - User.find_each do |user| - Rails.cache.delete("dawarich/user_#{user.id}_points_geocoded_stats") - end + keys = User.pluck(:id).map { |id| "dawarich/user_#{id}_points_geocoded_stats" } + Rails.cache.delete_multi(keys) if keys.any? end def delete_countries_cities_cache - User.find_each do |user| - Rails.cache.delete("dawarich/user_#{user.id}_countries_visited") - Rails.cache.delete("dawarich/user_#{user.id}_cities_visited") - end + keys = User.pluck(:id).flat_map do |id| + ["dawarich/user_#{id}_countries_visited", "dawarich/user_#{id}_cities_visited"] + end + Rails.cache.delete_multi(keys) if keys.any? endNote: This approach loads all user IDs into memory at once, which is more efficient for cache operations but uses more memory. Use
find_eachif you have a very large number of users.Also applies to: 31-42
app/mailers/users_mailer.rb (2)
5-6: Consider extracting comments to documentationWhile the inline comments are helpful, they might be better placed in class-level documentation or a separate documentation file. This would keep the code cleaner and allow for more detailed documentation.
+# UsersMailer handles all user-related email communications +# +# Email sending schedule: +# - welcome: Sent after user signs up +# - explore_features: Sent 2 days after user signs up +# - trial_expires_soon: Sent 2 days before trial expires +# - trial_expired: Sent when trial expires +# - post_trial_reminder_early: Sent 2 days after trial expires +# - post_trial_reminder_late: Sent 7 days after trial expires class UsersMailer < ApplicationMailer def welcome - # Sent after user signs up @user = params[:user] mail(to: @user.email, subject: 'Welcome to Dawarich!') end def explore_features - # Sent 2 days after user signs up @user = params[:user] mail(to: @user.email, subject: 'Explore Dawarich features!') endAlso applies to: 12-13, 19-20, 26-27, 33-34, 40-41
32-44: Consider adding error handling for missing user parameterThe new mailer methods don't validate that the user parameter exists before attempting to use it. This could lead to nil errors if called incorrectly.
def post_trial_reminder_early # Sent 2 days after trial expires @user = params[:user] + return unless @user mail(to: @user.email, subject: '🚀 Still interested in Dawarich? Subscribe now!') end def post_trial_reminder_late # Sent 7 days after trial expires @user = params[:user] + return unless @user mail(to: @user.email, subject: '📍 Your location data is waiting - Subscribe to Dawarich') endspec/jobs/users/mailer_sending_job_spec.rb (1)
114-116: Consider verifying the error is loggedSince the job now rescues
ActiveRecord::RecordNotFoundand logs it viaExceptionReporter, the test should verify that the error is properly logged rather than just checking it doesn't raise.+ it 'logs the missing user error' do + user.destroy + + expect(ExceptionReporter).to receive(:report).with( + a_string_matching(/User .* not found/), + hash_including(email_type: 'welcome') + ) + + described_class.perform_now(user.id, 'welcome') + endspec/services/cache/clean_spec.rb (1)
69-88: Test is overly complex and doesn't accurately test the real scenarioThe test that "handles users being added during execution gracefully" uses stubbing in a way that doesn't accurately represent what would happen in production. The stub creates a complex side effect that wouldn't occur with the real
find_eachimplementation.Consider simplifying or removing this test since:
find_eachuses database cursors that wouldn't include newly created records mid-iteration- The current stubbing creates an artificial scenario that wouldn't happen in practice
If you want to keep a concurrency test, consider:
- it 'handles users being added during execution gracefully' do - # Create a user that will be found during the cleaning process - user3 = nil - - allow(User).to receive(:find_each).and_yield(user1).and_yield(user2) do |&block| - # Create a new user while iterating - this should not cause errors - user3 = create(:user) - Rails.cache.write("dawarich/user_#{user3.id}_years_tracked", { 2023 => ['May'] }) - Rails.cache.write("dawarich/user_#{user3.id}_points_geocoded_stats", { geocoded: 1, without_data: 0 }) - - # Continue with the original block - [user1, user2].each(&block) - end - - expect { described_class.call }.not_to raise_error - - # The new user's cache should still exist since it wasn't processed - expect(Rails.cache.exist?("dawarich/user_#{user3.id}_years_tracked")).to be true - expect(Rails.cache.exist?("dawarich/user_#{user3.id}_points_geocoded_stats")).to be true - end + it 'only cleans cache for users that existed at the start of execution' do + # Verify it processes existing users + expect { described_class.call }.not_to raise_error + + # Create a new user after cleaning + user3 = create(:user) + Rails.cache.write("dawarich/user_#{user3.id}_years_tracked", { 2023 => ['May'] }) + + # Verify new user's cache remains until next clean + expect(Rails.cache.exist?("dawarich/user_#{user3.id}_years_tracked")).to be true + endapp/queries/stats_query.rb (1)
28-29: Consider using PostGIS functions for more efficient geocoding statistics.Since the app leverages PostGIS (as per coding guidelines), consider using PostGIS-specific functions for geocoding queries. For example, you could check if
lonlatis populated usingST_IsEmptyor similar spatial predicates rather than checkingreverse_geocoded_attimestamps.app/models/user.rb (1)
172-183: Consider making uncached methods public.The
countries_visited_uncachedandcities_visited_uncachedmethods are currently private but may be useful for cache preheating jobs or administrative functions. Consider making them public to avoid having to usesendto access them from other parts of the codebase.spec/jobs/bulk_stats_calculating_job_spec.rb (1)
26-27: Consider using database cleaner strategy instead of manual cleanup.The manual cleanup with
User.where.not(id: [...]).destroy_allcould be fragile if other test data needs to persist. Consider using RSpec's database cleaner with a transaction or truncation strategy for more reliable test isolation.Also applies to: 72-73
app/views/stats/index.html.erb (1)
96-101: Consider extracting the color palette to a configuration file.The hardcoded color array could be moved to a constant or configuration file for reusability across other charts and easier maintenance.
Define in a helper or configuration:
# app/helpers/charts_helper.rb CHART_COLOR_PALETTE = [ '#397bb5', '#5A4E9D', '#3B945E', '#7BC96F', '#FFD54F', '#FFA94D', '#FF6B6B', '#FF8C42', '#C97E4F', '#8B4513', '#5A2E2E', '#265d7d' ].freezeThen use in the view:
- colors: [ - '#397bb5', '#5A4E9D', '#3B945E', - '#7BC96F', '#FFD54F', '#FFA94D', - '#FF6B6B', '#FF8C42', '#C97E4F', - '#8B4513', '#5A2E2E', '#265d7d' - ] + colors: CHART_COLOR_PALETTEapp/helpers/stats_helper.rb (2)
140-172: Consider performance optimization for quietest week calculation.The method iterates through each day of the month with
each_cons(7), which could be inefficient for months with many data points. Also, the range calculation(start_date..end_date)might include days outside the actual month.def quietest_week(stat) return 'N/A' if stat.daily_distance.empty? - # Create a hash with date as key and distance as value distance_by_date = stat.daily_distance.to_h.transform_keys do |timestamp| Time.at(timestamp).in_time_zone(stat.user.timezone || 'UTC').to_date end - # Initialize variables to track the quietest week quietest_start_date = nil quietest_distance = Float::INFINITY - # Iterate through each day of the month to find the quietest week - start_date = distance_by_date.keys.min.beginning_of_month - end_date = distance_by_date.keys.max.end_of_month + # Use actual data range instead of full month to avoid unnecessary iterations + start_date = distance_by_date.keys.min + end_date = distance_by_date.keys.max + + # Ensure we have enough days for a week + return 'N/A' if (end_date - start_date).to_i < 6 (start_date..end_date).each_cons(7) do |week| week_distance = week.sum { |date| distance_by_date[date] || 0 } if week_distance < quietest_distance quietest_distance = week_distance quietest_start_date = week.first end end return 'N/A' unless quietest_start_date quietest_end_date = quietest_start_date + 6.days start_str = quietest_start_date.strftime('%b %d') end_str = quietest_end_date.strftime('%b %d') "#{start_str} - #{end_str}" end
174-232: Consider extracting month mappings to constants.The month-based case statements in
month_icon,month_color,month_gradient_classes, andmonth_bg_imagecould be extracted to constants for better maintainability and testability.+ MONTH_ICONS = { + (1..2) => 'snowflake', + 12 => 'snowflake', + (3..5) => 'flower', + (6..8) => 'tree-palm', + (9..11) => 'leaf' + }.freeze + + MONTH_COLORS = { + 1 => '#397bb5', 2 => '#5A4E9D', 3 => '#3B945E', + 4 => '#7BC96F', 5 => '#FFD54F', 6 => '#FFA94D', + 7 => '#FF6B6B', 8 => '#FF8C42', 9 => '#C97E4F', + 10 => '#8B4513', 11 => '#5A2E2E', 12 => '#265d7d' + }.freeze + def month_icon(stat) - case stat.month - when 1..2, 12 then 'snowflake' - when 3..5 then 'flower' - when 6..8 then 'tree-palm' - when 9..11 then 'leaf' - end + MONTH_ICONS.find { |range, _| range.cover?(stat.month) }&.last end def month_color(stat) - case stat.month - when 1 then '#397bb5' - # ... other cases - end + MONTH_COLORS[stat.month] end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
CHANGELOG.md(1 hunks)app/helpers/application_helper.rb(0 hunks)app/helpers/stats_helper.rb(1 hunks)app/jobs/bulk_stats_calculating_job.rb(1 hunks)app/jobs/cache/preheating_job.rb(1 hunks)app/jobs/users/mailer_sending_job.rb(1 hunks)app/mailers/users_mailer.rb(1 hunks)app/models/user.rb(3 hunks)app/queries/stats_query.rb(1 hunks)app/services/cache/clean.rb(2 hunks)app/views/stats/index.html.erb(4 hunks)app/views/users_mailer/explore_features.html.erb(1 hunks)app/views/users_mailer/explore_features.text.erb(1 hunks)app/views/users_mailer/post_trial_reminder_early.html.erb(1 hunks)app/views/users_mailer/post_trial_reminder_early.text.erb(1 hunks)app/views/users_mailer/post_trial_reminder_late.html.erb(1 hunks)app/views/users_mailer/post_trial_reminder_late.text.erb(1 hunks)app/views/users_mailer/trial_expired.html.erb(2 hunks)app/views/users_mailer/trial_expired.text.erb(2 hunks)app/views/users_mailer/trial_expires_soon.html.erb(2 hunks)app/views/users_mailer/trial_expires_soon.text.erb(2 hunks)app/views/users_mailer/welcome.html.erb(2 hunks)app/views/users_mailer/welcome.text.erb(2 hunks)config/initializers/cache_jobs.rb(0 hunks)db/migrate/20250905120121_add_user_country_composite_index_to_points.rb(1 hunks)spec/jobs/bulk_stats_calculating_job_spec.rb(1 hunks)spec/jobs/cache/preheating_job_spec.rb(1 hunks)spec/jobs/users/mailer_sending_job_spec.rb(1 hunks)spec/mailers/users_mailer_spec.rb(2 hunks)spec/queries/stats_query_spec.rb(7 hunks)spec/requests/api/v1/subscriptions_spec.rb(1 hunks)spec/services/cache/clean_spec.rb(1 hunks)
💤 Files with no reviewable changes (2)
- config/initializers/cache_jobs.rb
- app/helpers/application_helper.rb
✅ Files skipped from review due to trivial changes (1)
- app/views/users_mailer/trial_expires_soon.html.erb
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/requests/api/v1/subscriptions_spec.rb
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop with Rails extensions to lint Ruby code
Files:
db/migrate/20250905120121_add_user_country_composite_index_to_points.rbapp/services/cache/clean.rbapp/mailers/users_mailer.rbapp/jobs/bulk_stats_calculating_job.rbspec/jobs/cache/preheating_job_spec.rbapp/jobs/cache/preheating_job.rbspec/mailers/users_mailer_spec.rbapp/jobs/users/mailer_sending_job.rbspec/services/cache/clean_spec.rbspec/queries/stats_query_spec.rbapp/queries/stats_query.rbspec/jobs/bulk_stats_calculating_job_spec.rbapp/helpers/stats_helper.rbspec/jobs/users/mailer_sending_job_spec.rbapp/models/user.rb
db/migrate/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
db/migrate/**/*.rb: Use Strong Migrations and follow its recommendations for safe database changes
Add appropriate database indexes for geographic queries to ensure performance
Files:
db/migrate/20250905120121_add_user_country_composite_index_to_points.rb
app/{controllers,services}/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use Sidekiq for any potentially long-running operations instead of performing them inline
Files:
app/services/cache/clean.rb
app/{models,queries}/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Leverage PostGIS (database-side) functions for geographic calculations instead of Ruby-based solutions
Files:
app/queries/stats_query.rbapp/models/user.rb
🧠 Learnings (2)
📚 Learning: 2025-09-04T20:15:35.655Z
Learnt from: CR
PR: Freika/dawarich#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T20:15:35.655Z
Learning: Applies to db/migrate/**/*.rb : Add appropriate database indexes for geographic queries to ensure performance
Applied to files:
db/migrate/20250905120121_add_user_country_composite_index_to_points.rb
📚 Learning: 2025-09-04T20:15:35.655Z
Learnt from: CR
PR: Freika/dawarich#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T20:15:35.655Z
Learning: Applies to app/models/point.rb : Use the `lonlat` column in the `points` table for geographic calculations
Applied to files:
app/queries/stats_query.rb
🧬 Code graph analysis (8)
db/migrate/20250905120121_add_user_country_composite_index_to_points.rb (3)
db/migrate/20250723164055_add_track_generation_composite_index.rb (1)
change(4-8)db/migrate/20250515192211_add_country_id_to_points.rb (1)
change(6-8)db/migrate/20250728191359_add_country_name_to_points.rb (1)
change(4-9)
app/mailers/users_mailer.rb (1)
spec/mailers/previews/users_mailer_preview.rb (3)
explore_features(8-10)trial_expires_soon(12-14)trial_expired(16-18)
spec/jobs/cache/preheating_job_spec.rb (1)
app/jobs/cache/preheating_job.rb (1)
perform(6-32)
spec/mailers/users_mailer_spec.rb (1)
app/mailers/users_mailer.rb (7)
welcome(3-45)welcome(4-9)explore_features(11-16)trial_expires_soon(18-23)trial_expired(25-30)post_trial_reminder_early(32-37)post_trial_reminder_late(39-44)
spec/services/cache/clean_spec.rb (1)
app/services/cache/clean.rb (2)
call(3-44)call(5-13)
spec/queries/stats_query_spec.rb (1)
app/queries/stats_query.rb (1)
points_stats(8-18)
app/helpers/stats_helper.rb (6)
app/models/concerns/distance_convertible.rb (1)
convert_distance(41-52)app/models/user.rb (2)
safe_settings(34-36)timezone(130-132)app/services/users/safe_settings.rb (1)
distance_unit(115-117)app/services/stats/calculate_month.rb (2)
toponyms(61-71)distance(57-59)app/services/countries_and_cities.rb (1)
new(3-54)app/controllers/stats_controller.rb (1)
month(19-25)
app/models/user.rb (4)
app/services/countries_and_cities.rb (1)
new(3-54)app/models/stat.rb (1)
points(28-33)app/services/stats/calculate_month.rb (1)
points(46-55)app/models/point.rb (1)
without_raw_data(39-41)
🪛 ast-grep (0.38.6)
app/jobs/cache/preheating_job.rb
[warning] 13-17: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: Rails.cache.write(
"dawarich/user_#{user.id}_points_geocoded_stats",
StatsQuery.new(user).send(:cached_points_geocoded_stats),
expires_in: 1.day
)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 19-23: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: Rails.cache.write(
"dawarich/user_#{user.id}_countries_visited",
user.send(:countries_visited_uncached),
expires_in: 1.day
)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 25-29: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: Rails.cache.write(
"dawarich/user_#{user.id}_cities_visited",
user.send(:cities_visited_uncached),
expires_in: 1.day
)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
spec/jobs/users/mailer_sending_job_spec.rb
[warning] 114-114: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.perform_now(user.id, 'welcome')
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 RuboCop (1.76.1)
app/helpers/stats_helper.rb
[convention] 3-233: Module has too many lines. [177/100]
(Metrics/ModuleLength)
[convention] 10-47: Assignment Branch Condition size for countries_and_cities_stat_for_year is too high. [<11, 52, 14> 54.96/17]
(Metrics/AbcSize)
[convention] 10-47: Cyclomatic complexity for countries_and_cities_stat_for_year is too high. [13/7]
(Metrics/CyclomaticComplexity)
[convention] 10-47: Method has too many lines. [28/10]
(Metrics/MethodLength)
[convention] 10-47: Perceived complexity for countries_and_cities_stat_for_year is too high. [13/8]
(Metrics/PerceivedComplexity)
[convention] 120-138: Assignment Branch Condition size for peak_day is too high. [<6, 23, 5> 24.29/17]
(Metrics/AbcSize)
[convention] 120-138: Method has too many lines. [13/10]
(Metrics/MethodLength)
[convention] 140-172: Assignment Branch Condition size for quietest_week is too high. [<14, 24, 9> 29.21/17]
(Metrics/AbcSize)
[convention] 140-172: Cyclomatic complexity for quietest_week is too high. [9/7]
(Metrics/CyclomaticComplexity)
[convention] 140-172: Method has too many lines. [20/10]
(Metrics/MethodLength)
[convention] 140-172: Perceived complexity for quietest_week is too high. [9/8]
(Metrics/PerceivedComplexity)
[convention] 183-198: Cyclomatic complexity for month_color is too high. [13/7]
(Metrics/CyclomaticComplexity)
[convention] 183-198: Method has too many lines. [14/10]
(Metrics/MethodLength)
[convention] 200-215: Cyclomatic complexity for month_gradient_classes is too high. [13/7]
(Metrics/CyclomaticComplexity)
[convention] 200-215: Method has too many lines. [14/10]
(Metrics/MethodLength)
[convention] 217-232: Assignment Branch Condition size for month_bg_image is too high. [<0, 13, 12> 17.69/17]
(Metrics/AbcSize)
[convention] 217-232: Cyclomatic complexity for month_bg_image is too high. [13/7]
(Metrics/CyclomaticComplexity)
[convention] 217-232: Method has too many lines. [14/10]
(Metrics/MethodLength)
🔇 Additional comments (34)
db/migrate/20250905120121_add_user_country_composite_index_to_points.rb (1)
1-12: Excellent migration implementation for geographic performance optimization!This migration correctly follows Rails and Strong Migrations best practices:
- ✅ Uses
disable_ddl_transaction!to enable concurrent index creation- ✅ Uses
algorithm: :concurrentlyto avoid blocking table during index creation- ✅ Includes
if_not_exists: truefor safe re-execution- ✅ Provides descriptive index name
idx_points_user_country_name- ✅ Creates composite index on
(user_id, country_name)which perfectly supports user-scoped country queriesThe composite index aligns well with the existing geographic indexing strategy (as seen in previous migrations adding
country_nameandcountry_idcolumns) and will significantly improve performance for queries that filter points by user and group/filter by country.app/views/users_mailer/post_trial_reminder_late.text.erb (1)
1-27: LGTM! Well-crafted late reminder email.The email content strikes a good balance between being persistent and respectful, with clear messaging about data retention and a final reminder tone. The personalized greeting and sign-off maintain consistency with other email templates in the system.
app/views/users_mailer/post_trial_reminder_late.html.erb (1)
1-52: LGTM! Consistent HTML email design with proper UTM tracking.The HTML email template follows the established design patterns and includes proper UTM parameters for campaign tracking. The styling is consistent with other email templates and the content aligns well with the text version.
app/views/users_mailer/trial_expired.text.erb (1)
3-3: LGTM! Consistent personalization updates.The changes successfully personalize the email by introducing the sender and shifting from plural ("we") to singular ("I") voice, maintaining consistency with other email templates in the system.
Also applies to: 7-7, 22-22
app/views/users_mailer/welcome.html.erb (1)
19-19: LGTM! Consistent personalization across welcome email.The changes successfully personalize the welcome email with sender introduction and consistent singular voice throughout, aligning with the broader email template personalization effort across the system.
Also applies to: 21-21, 33-33
spec/mailers/users_mailer_spec.rb (1)
52-68: LGTM! Complete test coverage for new mailer methods.The new test contexts for
post_trial_reminder_earlyandpost_trial_reminder_lateproperly validate the subject lines and recipient emails, matching the expected behavior defined in the corresponding mailer methods.app/views/users_mailer/explore_features.html.erb (1)
20-20: LGTM! Consistent personalization and valuable feature addition.The changes successfully personalize the email with sender introduction and add a meaningful "Reliving your travels" feature that enhances the user's understanding of Dawarich's value proposition. The changes maintain consistency with other email templates in the system.
Also applies to: 22-22, 26-29
app/views/users_mailer/post_trial_reminder_early.text.erb (1)
1-25: LGTM! Well-structured email template with clear call-to-action.The template follows consistent messaging with the HTML counterpart and maintains good email best practices with clear structure, personalized greeting, and actionable content.
app/views/users_mailer/post_trial_reminder_early.html.erb (1)
1-50: LGTM! Professional email template with proper UTM tracking.The HTML template is well-structured with inline CSS for email client compatibility, proper UTM parameters for campaign tracking, and consistent messaging with the text counterpart.
app/views/users_mailer/welcome.text.erb (3)
3-3: LGTM! Consistent personalization with other email templates.The greeting update adds personal touch by introducing Evgenii as the sender, maintaining consistency across the email template suite.
5-5: LGTM! Voice consistency improvement.The change from "We're" to "I'm" aligns with the personalized sender approach established in the greeting.
15-15: LGTM! Consistent first-person voice.The contact information update maintains the personal voice by changing from "us" to "me" and adding the reply option for better user engagement.
spec/queries/stats_query_spec.rb (3)
6-7: Essential cache clearing for test isolation.Adding
Rails.cache.clearensures tests don't interfere with each other when testing caching behavior.
16-23: LGTM! Improved test readability.Converting to multi-line hash format makes the test expectations more readable and maintainable.
144-183: Comprehensive caching behavior validation.The new test suite properly validates the caching mechanism with appropriate expectations for cache keys, database interactions, and counter cache usage. The tests ensure proper isolation and verify both cache hits and misses.
app/views/users_mailer/trial_expired.html.erb (4)
20-20: LGTM! Consistent personalization.The greeting update maintains consistency with other email templates by introducing Evgenii as the sender.
26-26: LGTM! Voice consistency improvement.The pronoun change from "We" to "I" aligns with the personalized sender approach.
30-39: LGTM! Clear subscription benefits and call-to-action.The new subscription content block provides clear value proposition with well-structured benefits list and proper UTM tracking in the CTA link.
43-43: LGTM! Consistent first-person voice.The closing line maintains the personal tone established throughout the template.
app/views/users_mailer/trial_expires_soon.text.erb (4)
3-3: LGTM! Consistent personalization approach.The greeting update aligns with the personalization strategy across all email templates.
7-7: LGTM! Voice consistency improvement.The change to first-person voice maintains consistency with the sender personalization.
11-17: LGTM! Clear subscription value proposition.The new benefits section clearly outlines what users retain with a subscription, helping drive conversion with specific feature highlights.
22-22: LGTM! Improved contact accessibility.The contact line update maintains first-person voice and adds the reply option for better user engagement.
app/views/users_mailer/explore_features.text.erb (4)
3-3: LGTM! Consistent personalization.The greeting maintains consistency with the personalization strategy across email templates.
5-5: LGTM! Voice consistency improvement.The pronoun change aligns with the first-person sender approach.
9-23: LGTM! Enhanced feature showcase.The expanded feature list provides comprehensive overview of Dawarich capabilities, helping users discover and utilize more features during their trial period.
24-26: LGTM! Effective trial urgency messaging.The additional CTA and trial reminder create appropriate urgency while encouraging continued exploration.
spec/jobs/cache/preheating_job_spec.rb (1)
1-93: Comprehensive test coverage for cache preheating functionality.The test suite properly validates all aspects of the preheating job:
- Cache key generation and existence validation
- Data structure verification for different cache types
- Graceful handling of edge cases (users with no points)
- Proper test isolation with cache clearing
The tests align well with the production implementation in
app/jobs/cache/preheating_job.rband provide good coverage for the caching behavior.app/queries/stats_query.rb (1)
9-11: Good use of Rails caching for performance optimization.The caching implementation with a 1-day TTL is appropriate for geocoded stats that don't change frequently. The cache key properly includes the user ID for isolation.
app/models/user.rb (2)
39-46: LGTM! Well-structured caching for expensive queries.The caching implementation properly uses
without_raw_datato exclude potentially large raw data from memory, which is a good performance practice.
130-132: timezone intentionally returns the application timezoneConfirmed — this is intentional: app/models/user.rb (def timezone at ~L130) returns Time.zone.name; spec/models/user_spec.rb asserts user.timezone == Time.zone.name; app/models/stat.rb contains a “Future: Once user.timezone column exists … user.timezone.presence || Time.zone.name” comment; db/schema.rb contains users table but no timezone column. No change required now. If you add a per-user timezone (DB column or settings), update the method (or rename it to application_timezone until then).
app/jobs/users/mailer_sending_job.rb (2)
9-16: Excellent error handling and logging improvements.The addition of structured skip logging with detailed reasons enhances observability and debugging capabilities. The early return pattern is clean and efficient.
21-26: Good defensive programming with RecordNotFound handling.Properly handling missing user records prevents job failures and ensures graceful degradation. The logging provides adequate context for debugging.
spec/jobs/bulk_stats_calculating_job_spec.rb (1)
103-105: Good use of test doubles for isolation.The stubbing of
User.activeandUser.trialwithUser.noneeffectively tests the "no users" scenario without relying on database state.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
Gemfile (1)
48-48: Scope stackprof to dev/test and disable auto-require.Avoid loading profiler in production.
-gem 'stackprof' +group :development, :test do + gem 'stackprof', require: false +endapp/models/stat.rb (1)
63-78: Normalize allowed expirations; validate input; support week/permanent.Current case statement silently yields nil expires_at for unknown inputs and lacks “1w/1_week/permanent” support.
- def enable_sharing!(expiration: '1h') - expires_at = case expiration - when '1h' then 1.hour.from_now - when '12h' then 12.hours.from_now - when '24h' then 24.hours.from_now - end + EXPIRATION_DURATIONS = { + '1h' => 1.hour, '12h' => 12.hours, '24h' => 24.hours, + '1w' => 1.week, '1_week' => 1.week, 'permanent' => nil + }.freeze + + def enable_sharing!(expiration: '1h') + duration = EXPIRATION_DURATIONS[expiration] + raise ArgumentError, "Unknown expiration: #{expiration}" if duration.nil? && expiration != 'permanent' + expires_at = duration&.from_nowapp/helpers/stats_helper.rb (1)
63-72: Guard nil/zero average to avoid divide-by-zero/Infinity.Current guard skips nil; to_f.zero? path still divides by 0.0.
- def x_than_average_distance(stat, average_distance_this_year) - return '' if average_distance_this_year&.zero? + def x_than_average_distance(stat, average_distance_this_year) + return '' if average_distance_this_year.nil? || average_distance_this_year.to_f.zero?app/controllers/stats_controller.rb (1)
19-25: Validate params and guard nil averages to prevent bad states.to_i coerces invalid input to 0; average can be nil. Return 400 on invalid month, and compute average safely.
def month - @year = params[:year].to_i - @month = params[:month].to_i + @year = Integer(params[:year], exception: false) + @month = Integer(params[:month], exception: false) + return head :bad_request unless @year && @month && (1..12).cover?(@month) @stat = current_user.stats.find_by(year: @year, month: @month) - @previous_stat = current_user.stats.find_by(year: @year, month: @month - 1) if @month > 1 - @average_distance_this_year = current_user.stats.where(year: @year).average(:distance).to_i / 1000 + @previous_stat = current_user.stats.find_by(year: @year, month: @month - 1) if @month > 1 + avg_meters = current_user.stats.where(year: @year).average(:distance) + @average_distance_this_year = (avg_meters.to_f / 1000.0) endapp/queries/hexagon_query.rb (2)
52-55: Fix geometry/geography mismatch in ST_Intersects.You’re intersecting geography (lonlat) with geometry (bbox). Cast both to the same type; prefer geography to leverage the GiST index on lonlat.
- AND ST_Intersects( - lonlat, - (SELECT geom FROM bbox_geom)::geometry - ) + AND ST_Intersects( + lonlat, + (SELECT geom FROM bbox_geom)::geography + )Alternatively, cast lonlat to ::geometry and drop ::geography on bbox to keep everything geometry.
9-18: Coerce/validate inputs in initializer.Ensure numeric bounds/hex_size/user_id are typed, dates parseable; fail fast.
- def initialize(min_lon:, min_lat:, max_lon:, max_lat:, hex_size:, user_id: nil, start_date: nil, end_date: nil) - @min_lon = min_lon - @min_lat = min_lat - @max_lon = max_lon - @max_lat = max_lat - @hex_size = hex_size - @user_id = user_id - @start_date = start_date - @end_date = end_date + def initialize(min_lon:, min_lat:, max_lon:, max_lat:, hex_size:, user_id: nil, start_date: nil, end_date: nil) + @min_lon = Float(min_lon) + @min_lat = Float(min_lat) + @max_lon = Float(max_lon) + @max_lat = Float(max_lat) + raise ArgumentError, 'Invalid bbox (min must be < max)' unless @min_lon < @max_lon && @min_lat < @max_lat + @hex_size = Float(hex_size) + @user_id = user_id.nil? ? nil : Integer(user_id) + @start_date = start_date&.to_s + @end_date = end_date&.to_sapp/views/stats/_month.html.erb (3)
7-9: Removehtml_safeand build safe HTML fragmentsAvoid manual trust of a composed string; use
safe_joinwith the icon helper and plain text.- <%= "#{icon month_icon(stat)} #{Date::MONTHNAMES[month]} #{year}".html_safe %> + <%= safe_join([icon(month_icon(stat)), "#{Date::MONTHNAMES[month]} #{year}"]) %>Follow-up (optional): localize the month/year.
<%= safe_join([icon(month_icon(stat)), I18n.l(Date.new(year, month, 1), format: "%B %Y")]) %>
55-59: Do not expose API keys in the DOMLeaking
current_user.api_keyto the page is a high-risk secret exposure. Use session-backed requests or a server-side proxy instead.<div class="card bg-base-100 shadow-xl mb-8" data-controller="stat-page" - data-api-key="<%= current_user.api_key %>" data-year="<%= year %>" data-month="<%= month %>" data-self-hosted="<%= @self_hosted %>">Action items:
- Remove any JS reads of this data attribute and switch to same-origin authenticated fetches.
- Rotate any keys that may have been exposed.
245-252: Guard against nilstat.toponymsbefore iteratingThis block will raise when
stat.toponymsis nil (even if the earlier section handled the empty case).<div class="flex flex-wrap gap-2"> <span class="text-sm font-medium">Cities visited:</span> - <% stat.toponyms.each do |country| %> - <% country['cities'].each do |city| %> - <div class="badge badge-outline"><%= city['city'] %></div> - <% end %> - <% end %> + <% if stat.toponyms.present? %> + <% stat.toponyms.each do |country| %> + <% Array(country['cities']).each do |city| %> + <div class="badge badge-outline"><%= city['city'] %></div> + <% end %> + <% end %> + <% else %> + <span class="opacity-70">None</span> + <% end %> </div>
🧹 Nitpick comments (10)
app/helpers/stats_helper.rb (4)
10-47: Reduce complexity and add nil-safety for cities arrays.cities access can be nil; method is long. Extract helpers and guard nil.
- data[:cities] << _1.toponyms.flatten.flat_map { |t| t['cities'].map { |c| c['city'] } }.compact.uniq + data[:cities] << _1.toponyms.flatten.flat_map { |t| t['cities']&.map { |c| c['city'] } || [] }.compact.uniqIf you’re open, I can provide a small refactor splitting extraction/mapping into private helpers.
56-61: Consider consistent rounding (2 decimals) for distance display.UI reads better for non-integer miles/kilometers.
- value = Stat.convert_distance(stat.distance, distance_unit).round + value = Stat.convert_distance(stat.distance, distance_unit).round(2)
115-126: Align peak_day rounding with distance_traveled.Use 2 decimals for consistency.
- distance_value = Stat.convert_distance(peak[1], distance_unit).round + distance_value = Stat.convert_distance(peak[1], distance_unit).round(2)
162-203: Optional: replace long case statements with lookup tables.Hash/array maps will shrink code, ease maintenance, and satisfy RuboCop complexity cops.
I can send a follow-up patch introducing MONTH_ICON/COLOR/GRADIENT/BG maps and simple lookups.
Also applies to: 205-220
app/jobs/cache/preheating_job.rb (1)
14-30: LGTM: preheat caches without using send; keys and TTLs are clear.Consider batching with find_each(batch_size: N) if dataset is large and adding basic metrics (duration, miss rate) to observe effectiveness.
app/models/user.rb (1)
50-52: LGTM: cities_visited cached; consider mirroring without_raw_data for consistency (optional).app/queries/hexagon_query.rb (1)
98-117: Optional: use typed binds for exec_query.Rails adapters expect typed QueryAttribute binds; current plain values usually work but typed binds are safer across adapters.
I can provide a small helper to build QueryAttribute binds for floats/ints/timestamps and simplify current_param_index math.
Also applies to: 119-139
app/views/stats/_month.html.erb (3)
55-59: Use Stimulus values for typed data instead of ad-hoc attributesThis gives you automatic parsing (numbers/booleans) and clearer APIs.
- data-year="<%= year %>" - data-month="<%= month %>" - data-self-hosted="<%= @self_hosted %>"> + data-stat-page-year-value="<%= year %>" + data-stat-page-month-value="<%= month %>" + data-stat-page-self-hosted-value="<%= @self_hosted %>">
67-73: Improve toggle accessibility witharia-pressedExpose state to assistive tech; toggle these attrs in the controller.
- <button class="btn btn-sm btn-outline btn-active" data-stat-page-target="heatmapBtn" data-action="click->stat-page#toggleHeatmap"> + <button class="btn btn-sm btn-outline btn-active" aria-pressed="true" data-stat-page-target="heatmapBtn" data-action="click->stat-page#toggleHeatmap"> <%= icon 'flame' %> Heatmap </button> - <button class="btn btn-sm btn-outline" data-stat-page-target="pointsBtn" data-action="click->stat-page#togglePoints"> + <button class="btn btn-sm btn-outline" aria-pressed="false" data-stat-page-target="pointsBtn" data-action="click->stat-page#togglePoints"> <%= icon 'map-pin' %> Points </button>
112-154: Consider fragment caching for expensive sectionsThe chart and stats blocks can be cached by user/month to cut render time.
Example:
<% cache ["stats/month", current_user.id, year, month, stat.cache_key_with_version] do %> ... Daily Activity card ... <% end %>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Gemfile(2 hunks)app/controllers/application_controller.rb(3 hunks)app/controllers/settings/users_controller.rb(5 hunks)app/controllers/stats_controller.rb(1 hunks)app/helpers/stats_helper.rb(1 hunks)app/jobs/cache/preheating_job.rb(1 hunks)app/models/stat.rb(2 hunks)app/models/user.rb(3 hunks)app/queries/hexagon_query.rb(1 hunks)app/queries/stats_query.rb(1 hunks)app/views/stats/_month.html.erb(1 hunks)config/routes.rb(2 hunks)spec/jobs/users/mailer_sending_job_spec.rb(1 hunks)spec/services/cache/clean_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/controllers/settings/users_controller.rb
- spec/services/cache/clean_spec.rb
- app/controllers/application_controller.rb
- config/routes.rb
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop with Rails extensions to lint Ruby code
Files:
app/queries/stats_query.rbapp/models/user.rbapp/controllers/stats_controller.rbapp/helpers/stats_helper.rbapp/jobs/cache/preheating_job.rbapp/models/stat.rbapp/queries/hexagon_query.rbspec/jobs/users/mailer_sending_job_spec.rb
app/{models,queries}/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Leverage PostGIS (database-side) functions for geographic calculations instead of Ruby-based solutions
Files:
app/queries/stats_query.rbapp/models/user.rbapp/models/stat.rbapp/queries/hexagon_query.rb
app/{controllers,services}/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use Sidekiq for any potentially long-running operations instead of performing them inline
Files:
app/controllers/stats_controller.rb
🧠 Learnings (3)
📚 Learning: 2025-09-04T20:15:35.655Z
Learnt from: CR
PR: Freika/dawarich#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T20:15:35.655Z
Learning: Applies to app/{models,queries}/**/*.rb : Leverage PostGIS (database-side) functions for geographic calculations instead of Ruby-based solutions
Applied to files:
app/queries/stats_query.rbapp/queries/hexagon_query.rb
📚 Learning: 2025-09-04T20:15:35.655Z
Learnt from: CR
PR: Freika/dawarich#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T20:15:35.655Z
Learning: Applies to db/migrate/**/*.rb : Add appropriate database indexes for geographic queries to ensure performance
Applied to files:
app/queries/hexagon_query.rb
📚 Learning: 2025-09-04T20:15:35.655Z
Learnt from: CR
PR: Freika/dawarich#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T20:15:35.655Z
Learning: Applies to app/models/point.rb : Use the `lonlat` column in the `points` table for geographic calculations
Applied to files:
app/queries/hexagon_query.rb
🧬 Code graph analysis (5)
app/models/user.rb (2)
app/models/stat.rb (1)
points(28-33)app/models/point.rb (1)
without_raw_data(39-41)
app/helpers/stats_helper.rb (6)
app/models/concerns/distance_convertible.rb (1)
convert_distance(41-52)app/models/user.rb (2)
safe_settings(34-36)timezone(130-132)app/services/users/safe_settings.rb (1)
distance_unit(115-117)app/services/stats/calculate_month.rb (2)
toponyms(61-71)distance(57-59)app/services/countries_and_cities.rb (1)
new(3-54)app/controllers/stats_controller.rb (1)
month(19-25)
app/jobs/cache/preheating_job.rb (2)
app/queries/stats_query.rb (1)
cached_points_geocoded_stats(20-40)app/models/user.rb (2)
countries_visited_uncached(134-141)cities_visited_uncached(143-145)
app/models/stat.rb (3)
app/controllers/stats_controller.rb (1)
month(19-25)app/services/stats/calculate_month.rb (1)
points(46-55)app/models/trip.rb (1)
points(20-22)
app/queries/hexagon_query.rb (3)
app/queries/stats_query.rb (2)
initialize(3-45)initialize(4-6)app/services/maps/hexagon_grid.rb (2)
initialize(25-36)call(38-42)app/services/stats/calculate_month.rb (1)
end_timestamp(28-30)
🪛 RuboCop (1.76.1)
app/controllers/stats_controller.rb
[convention] 19-25: Assignment Branch Condition size for month is too high. [<5, 19, 2> 19.75/17]
(Metrics/AbcSize)
app/helpers/stats_helper.rb
[convention] 3-221: Module has too many lines. [167/100]
(Metrics/ModuleLength)
[convention] 10-47: Assignment Branch Condition size for countries_and_cities_stat_for_year is too high. [<11, 52, 14> 54.96/17]
(Metrics/AbcSize)
[convention] 10-47: Cyclomatic complexity for countries_and_cities_stat_for_year is too high. [13/7]
(Metrics/CyclomaticComplexity)
[convention] 10-47: Method has too many lines. [28/10]
(Metrics/MethodLength)
[convention] 10-47: Perceived complexity for countries_and_cities_stat_for_year is too high. [13/8]
(Metrics/PerceivedComplexity)
[convention] 115-126: Assignment Branch Condition size for peak_day is too high. [<5, 20, 2> 20.71/17]
(Metrics/AbcSize)
[convention] 128-160: Assignment Branch Condition size for quietest_week is too high. [<14, 24, 9> 29.21/17]
(Metrics/AbcSize)
[convention] 128-160: Cyclomatic complexity for quietest_week is too high. [9/7]
(Metrics/CyclomaticComplexity)
[convention] 128-160: Method has too many lines. [20/10]
(Metrics/MethodLength)
[convention] 128-160: Perceived complexity for quietest_week is too high. [9/8]
(Metrics/PerceivedComplexity)
[convention] 171-186: Cyclomatic complexity for month_color is too high. [13/7]
(Metrics/CyclomaticComplexity)
[convention] 171-186: Method has too many lines. [14/10]
(Metrics/MethodLength)
[convention] 188-203: Cyclomatic complexity for month_gradient_classes is too high. [13/7]
(Metrics/CyclomaticComplexity)
[convention] 188-203: Method has too many lines. [14/10]
(Metrics/MethodLength)
[convention] 205-220: Assignment Branch Condition size for month_bg_image is too high. [<0, 13, 12> 17.69/17]
(Metrics/AbcSize)
[convention] 205-220: Cyclomatic complexity for month_bg_image is too high. [13/7]
(Metrics/CyclomaticComplexity)
[convention] 205-220: Method has too many lines. [14/10]
(Metrics/MethodLength)
app/models/stat.rb
[convention] 63-78: Method has too many lines. [13/10]
(Metrics/MethodLength)
[convention] 90-116: Assignment Branch Condition size for calculate_data_bounds is too high. [<5, 28, 1> 28.46/17]
(Metrics/AbcSize)
[convention] 90-116: Method has too many lines. [21/10]
(Metrics/MethodLength)
app/queries/hexagon_query.rb
[convention] 3-148: Class has too many lines. [122/100]
(Metrics/ClassLength)
[convention] 9-9: Avoid parameter lists longer than 5 parameters. [8/5]
(Metrics/ParameterLists)
[convention] 98-117: Method has too many lines. [12/10]
(Metrics/MethodLength)
[convention] 119-139: Method has too many lines. [15/10]
(Metrics/MethodLength)
🪛 ast-grep (0.38.6)
app/jobs/cache/preheating_job.rb
[warning] 13-17: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: Rails.cache.write(
"dawarich/user_#{user.id}_points_geocoded_stats",
StatsQuery.new(user).cached_points_geocoded_stats,
expires_in: 1.day
)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 19-23: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: Rails.cache.write(
"dawarich/user_#{user.id}_countries_visited",
user.countries_visited_uncached,
expires_in: 1.day
)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 25-29: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: Rails.cache.write(
"dawarich/user_#{user.id}_cities_visited",
user.cities_visited_uncached,
expires_in: 1.day
)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
spec/jobs/users/mailer_sending_job_spec.rb
[warning] 114-114: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.perform_now(user.id, 'welcome')
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 Brakeman (7.0.2)
app/views/stats/_month.html.erb
[weak] 8-8: Unescaped model attribute
Type: Cross-Site Scripting
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/cross_site_scripting
(Cross-Site Scripting)
🔇 Additional comments (17)
spec/jobs/users/mailer_sending_job_spec.rb (1)
111-117: Strengthen deleted-user spec: assert no mail is sent and verify reporter callThe job calls UsersMailer.with(...).public_send(...).deliver_later and logs skips with ExceptionReporter.call(...), so the spec should assert the mailer is not invoked and ExceptionReporter.call is invoked with the job name and a message including the user id and email type.
Location: spec/jobs/users/mailer_sending_job_spec.rb (the destroyed-user example).
Suggested expectations to add:
- expect(UsersMailer).not_to receive(:with)
- expect(ExceptionReporter).to receive(:call).with('Users::MailerSendingJob', a_string_including("User with ID #{user.id}", 'Skipping welcome email'))
- expect { described_class.perform_now(user.id, 'welcome') }.not_to raise_error
Likely an incorrect or invalid review comment.
Gemfile (4)
22-22: Good: JWT pinned to safe minor.Pinning to ~> 2.8 avoids surprise major upgrades.
38-38: rqrcode version looks fine.~> 3.0 is stable and widely used.
32-32: Confirm rails_icons licensing/assets pipeline integration.Ensure initializer and asset pipeline are present; precompile step includes icon assets.
12-12: Verify AWS SDK component version compatibility — manual check requiredSandbox lacks bundler ("bundle: command not found"); run locally and paste the output of:
bundle lock --update aws-sdk-core aws-sdk-kms aws-sdk-s3 && bundle exec ruby -e "require 'aws-sdk-s3'; puts Aws::S3::Client.api.version"Ensure aws-sdk-core (
> 3.215.1), aws-sdk-kms (> 1.96.0), and aws-sdk-s3 (~> 1.177.0) are mutually compatible; if resolution warnings appear, align via a single aws-sdk-* meta gem or bump core first. Location: Gemfile line 12app/models/stat.rb (4)
35-37: LGTM: sharing_enabled? is simple and clear.
39-53: LGTM: robust expiration parsing and fallbacks.Safely handles nil/invalid timestamps and “permanent”.
55-57: LGTM: public_accessible? composition reads well.
120-122: LGTM: generate_sharing_uuid is safe on create.app/queries/stats_query.rb (2)
8-18: LGTM: points_stats uses cached geocode stats and counter cache total.
20-40: LGTM: sanitized SQL and integer casting for geocode stats.app/models/user.rb (5)
30-30: LGTM: active_or_trial scope improves job targeting.
39-46: LGTM: countries_visited caching with without_raw_data and compaction.
130-133: LGTM: timezone accessor centralizes TZ source.
134-146: LGTM: uncached helpers enable preheating without reflection.
177-183: Confirm mailers/templates exist for new post-trial reminders.Avoid enqueuing missing jobs in non-self-hosted flows.
app/views/stats/_month.html.erb (1)
2-4: No action needed — helper returns server-controlled asset URLs
app/helpers/stats_helper.rb: month_bg_image(stat) maps months to static asset paths using image_url('backgrounds/months/...'), so the URL is asset-managed (not user-supplied) and safe for inline CSS.
| def quietest_week(stat) | ||
| return 'N/A' if stat.daily_distance.empty? | ||
|
|
||
| # Create a hash with date as key and distance as value | ||
| distance_by_date = stat.daily_distance.to_h.transform_keys do |timestamp| | ||
| Time.at(timestamp).in_time_zone(stat.user.timezone || 'UTC').to_date | ||
| end | ||
|
|
||
| # Initialize variables to track the quietest week | ||
| quietest_start_date = nil | ||
| quietest_distance = Float::INFINITY | ||
|
|
||
| # Iterate through each day of the month to find the quietest week | ||
| start_date = distance_by_date.keys.min.beginning_of_month | ||
| end_date = distance_by_date.keys.max.end_of_month | ||
|
|
||
| (start_date..end_date).each_cons(7) do |week| | ||
| week_distance = week.sum { |date| distance_by_date[date] || 0 } | ||
|
|
||
| if week_distance < quietest_distance | ||
| quietest_distance = week_distance | ||
| quietest_start_date = week.first | ||
| end | ||
| end | ||
|
|
||
| return 'N/A' unless quietest_start_date | ||
|
|
||
| quietest_end_date = quietest_start_date + 6.days | ||
| start_str = quietest_start_date.strftime('%b %d') | ||
| end_str = quietest_end_date.strftime('%b %d') | ||
|
|
||
| "#{start_str} - #{end_str}" | ||
| end |
There was a problem hiding this comment.
Fix quietest_week: daily_distance keys are day-of-month, not UNIX timestamps.
Current code treats keys as epoch seconds → wrong dates. Compute dates from year/month/day and slide a 7-day window.
- def quietest_week(stat)
- return 'N/A' if stat.daily_distance.empty?
-
- # Create a hash with date as key and distance as value
- distance_by_date = stat.daily_distance.to_h.transform_keys do |timestamp|
- Time.at(timestamp).in_time_zone(stat.user.timezone || 'UTC').to_date
- end
- ...
- (start_date..end_date).each_cons(7) do |week|
- week_distance = week.sum { |date| distance_by_date[date] || 0 }
- ...
- end
- ...
- "#{start_str} - #{end_str}"
- end
+ def quietest_week(stat)
+ return 'N/A' if stat.daily_distance.empty?
+
+ tz = stat.user.timezone
+ days_in_month = Time.find_zone(tz).local(stat.year, stat.month, 1).end_of_month.day
+ distance_by_day = stat.daily_distance.to_h # { day_of_month => meters }
+
+ # Build per-date distances within the month
+ dates = (1..days_in_month).map { |d| Date.new(stat.year, stat.month, d) }
+ distance_by_date = dates.to_h { |d| [d, distance_by_day[d.day] || 0] }
+
+ quietest_start = nil
+ quietest_total = Float::INFINITY
+ dates.each_cons(7) do |week|
+ total = week.sum { |d| distance_by_date[d] }
+ if total < quietest_total
+ quietest_total = total
+ quietest_start = week.first
+ end
+ end
+
+ return 'N/A' unless quietest_start
+ "#{quietest_start.strftime('%b %d')} - #{(quietest_start + 6).strftime('%b %d')}"
+ endCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 RuboCop (1.76.1)
[convention] 128-160: Assignment Branch Condition size for quietest_week is too high. [<14, 24, 9> 29.21/17]
(Metrics/AbcSize)
[convention] 128-160: Cyclomatic complexity for quietest_week is too high. [9/7]
(Metrics/CyclomaticComplexity)
[convention] 128-160: Method has too many lines. [20/10]
(Metrics/MethodLength)
[convention] 128-160: Perceived complexity for quietest_week is too high. [9/8]
(Metrics/PerceivedComplexity)
| def calculate_data_bounds | ||
| start_date = Date.new(year, month, 1).beginning_of_day | ||
| end_date = start_date.end_of_month.end_of_day | ||
|
|
||
| points_relation = user.points.where(timestamp: start_date.to_i..end_date.to_i) | ||
| point_count = points_relation.count | ||
|
|
||
| return nil if point_count.zero? | ||
|
|
||
| bounds_result = ActiveRecord::Base.connection.exec_query( | ||
| "SELECT MIN(latitude) as min_lat, MAX(latitude) as max_lat, | ||
| MIN(longitude) as min_lng, MAX(longitude) as max_lng | ||
| FROM points | ||
| WHERE user_id = $1 | ||
| AND timestamp BETWEEN $2 AND $3", | ||
| 'data_bounds_query', | ||
| [user.id, start_date.to_i, end_date.to_i] | ||
| ).first | ||
|
|
||
| { | ||
| min_lat: bounds_result['min_lat'].to_f, | ||
| max_lat: bounds_result['max_lat'].to_f, | ||
| min_lng: bounds_result['min_lng'].to_f, | ||
| max_lng: bounds_result['max_lng'].to_f, | ||
| point_count: point_count | ||
| } | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use PostGIS extent on lonlat; fix binds; avoid latitude/longitude columns.
Rely on lonlat + ST_Extent for performance and correctness; current query assumes latitude/longitude columns and uses untyped binds.
- bounds_result = ActiveRecord::Base.connection.exec_query(
- "SELECT MIN(latitude) as min_lat, MAX(latitude) as max_lat,
- MIN(longitude) as min_lng, MAX(longitude) as max_lng
- FROM points
- WHERE user_id = $1
- AND timestamp BETWEEN $2 AND $3",
- 'data_bounds_query',
- [user.id, start_date.to_i, end_date.to_i]
- ).first
+ sql = <<~SQL
+ SELECT
+ ST_YMin(extent) AS min_lat,
+ ST_YMax(extent) AS max_lat,
+ ST_XMin(extent) AS min_lng,
+ ST_XMax(extent) AS max_lng
+ FROM (
+ SELECT ST_Extent(lonlat::geometry) AS extent
+ FROM points
+ WHERE user_id = $1
+ AND timestamp BETWEEN $2 AND $3
+ ) e
+ SQL
+ binds = [
+ ActiveRecord::Relation::QueryAttribute.new('user_id', user.id, ActiveRecord::Type::Integer.new),
+ ActiveRecord::Relation::QueryAttribute.new('start_ts', start_date.to_i, ActiveRecord::Type::Integer.new),
+ ActiveRecord::Relation::QueryAttribute.new('end_ts', end_date.to_i, ActiveRecord::Type::Integer.new),
+ ]
+ bounds_result = ActiveRecord::Base.connection.exec_query(sql, 'data_bounds_query', binds).first- If lonlat is geography, cast bbox with ::geometry only for ST_Extent; ensure GiST index exists on lonlat. Consider an index on (user_id, timestamp) if not present.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def calculate_data_bounds | |
| start_date = Date.new(year, month, 1).beginning_of_day | |
| end_date = start_date.end_of_month.end_of_day | |
| points_relation = user.points.where(timestamp: start_date.to_i..end_date.to_i) | |
| point_count = points_relation.count | |
| return nil if point_count.zero? | |
| bounds_result = ActiveRecord::Base.connection.exec_query( | |
| "SELECT MIN(latitude) as min_lat, MAX(latitude) as max_lat, | |
| MIN(longitude) as min_lng, MAX(longitude) as max_lng | |
| FROM points | |
| WHERE user_id = $1 | |
| AND timestamp BETWEEN $2 AND $3", | |
| 'data_bounds_query', | |
| [user.id, start_date.to_i, end_date.to_i] | |
| ).first | |
| { | |
| min_lat: bounds_result['min_lat'].to_f, | |
| max_lat: bounds_result['max_lat'].to_f, | |
| min_lng: bounds_result['min_lng'].to_f, | |
| max_lng: bounds_result['max_lng'].to_f, | |
| point_count: point_count | |
| } | |
| end | |
| def calculate_data_bounds | |
| start_date = Date.new(year, month, 1).beginning_of_day | |
| end_date = start_date.end_of_month.end_of_day | |
| points_relation = user.points.where(timestamp: start_date.to_i..end_date.to_i) | |
| point_count = points_relation.count | |
| return nil if point_count.zero? | |
| sql = <<~SQL | |
| SELECT | |
| ST_YMin(extent) AS min_lat, | |
| ST_YMax(extent) AS max_lat, | |
| ST_XMin(extent) AS min_lng, | |
| ST_XMax(extent) AS max_lng | |
| FROM ( | |
| SELECT ST_Extent(lonlat::geometry) AS extent | |
| FROM points | |
| WHERE user_id = $1 | |
| AND timestamp BETWEEN $2 AND $3 | |
| ) e | |
| SQL | |
| binds = [ | |
| ActiveRecord::Relation::QueryAttribute.new('user_id', user.id, ActiveRecord::Type::Integer.new), | |
| ActiveRecord::Relation::QueryAttribute.new('start_ts', start_date.to_i, ActiveRecord::Type::Integer.new), | |
| ActiveRecord::Relation::QueryAttribute.new('end_ts', end_date.to_i, ActiveRecord::Type::Integer.new), | |
| ] | |
| bounds_result = ActiveRecord::Base.connection.exec_query(sql, 'data_bounds_query', binds).first | |
| { | |
| min_lat: bounds_result['min_lat'].to_f, | |
| max_lat: bounds_result['max_lat'].to_f, | |
| min_lng: bounds_result['min_lng'].to_f, | |
| max_lng: bounds_result['max_lng'].to_f, | |
| point_count: point_count | |
| } | |
| end |
🧰 Tools
🪛 RuboCop (1.76.1)
[convention] 90-116: Assignment Branch Condition size for calculate_data_bounds is too high. [<5, 28, 1> 28.46/17]
(Metrics/AbcSize)
[convention] 90-116: Method has too many lines. [21/10]
(Metrics/MethodLength)
| gem 'sentry-ruby' | ||
| gem 'sentry-rails' | ||
| gem 'stackprof' | ||
| gem 'sentry-ruby' |
There was a problem hiding this comment.
Remove duplicate Sentry gems; prefer only sentry-rails.
Including both sentry-rails and sentry-ruby can double-instrument and cause conflicts. sentry-rails already depends on sentry-ruby—keep just sentry-rails.
Apply:
gem 'sentry-rails'
-gem 'sentry-ruby'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gem 'sentry-ruby' |
🤖 Prompt for AI Agents
In Gemfile around line 43, you have added gem 'sentry-ruby' which duplicates
functionality provided by sentry-rails; remove the gem 'sentry-ruby' line so
only sentry-rails remains, then run bundle install (or bundle update) to update
the lockfile and ensure only the sentry-rails-provided sentry-ruby dependency is
used to avoid double-instrumentation.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/controllers/settings/users_controller.rb (1)
92-98: Redirecting inside validate_archive_file risks double render.
redirect_to ... and returninside a helper only exits that method; the action continues and will attempt another redirect, causing a DoubleRender error (masked by your rescue). Validate via a boolean helper and early-return from the action instead.Apply this diff to replace the validator:
- def validate_archive_file(archive_file) - unless ['application/zip', 'application/x-zip-compressed'].include?(archive_file.content_type) || - File.extname(archive_file.original_filename).downcase == '.zip' - - redirect_to edit_user_registration_path, alert: 'Please upload a valid ZIP file.' and return - end - end + def valid_archive_file?(archive_file) + ['application/zip', 'application/x-zip-compressed'].include?(archive_file.content_type) || + File.extname(archive_file.original_filename).downcase == '.zip' + endAnd update the import action (outside the changed hunk) accordingly:
# inside def import, after `archive_file = params[:archive]` unless valid_archive_file?(archive_file) redirect_to edit_user_registration_path, alert: 'Please upload a valid ZIP file.' return endapp/models/stat.rb (1)
28-33: Fix timestamp type mismatch in app/models/stat.rb — use integer range for DB querySchema shows points.timestamp is an integer, while timespan returns a DateTime range (spec). Change the query to compare integers so the DB column and bounds match.
- Location: app/models/stat.rb — points method (lines ~28–33); timespan remains DateTime (lines ~124–129).
- Suggested fix: replace .where(timestamp: timespan) with .where(timestamp: timespan.begin.to_i..timespan.end.to_i).
♻️ Duplicate comments (20)
Gemfile (1)
48-48: Scope stackprof to dev/test and avoid auto-require.Profilers shouldn’t load in production. Move to
group :development, :testand addrequire: false.Apply:
-gem 'stackprof'And inside the existing development/test group:
group :development, :test do + gem 'stackprof', require: false endapp/jobs/cache/preheating_job.rb (1)
14-18: Nice: stopped usingsendand call public methods directly.Encapsulation issue from the earlier review is resolved.
Also applies to: 20-24, 26-30
app/controllers/settings/users_controller.rb (1)
4-6: Run authenticate_user! before self-hosted/admin checks.
Ensures unauthenticated users are redirected to sign-in instead of seeing “not authorized.” Same suggestion as earlier review.Apply this diff:
-class Settings::UsersController < ApplicationController - before_action :authenticate_self_hosted!, except: %i[export import] - before_action :authenticate_user! - before_action :authenticate_admin!, except: %i[export import] +class Settings::UsersController < ApplicationController + before_action :authenticate_user! + before_action :authenticate_self_hosted!, except: %i[export import] + before_action :authenticate_admin!, except: %i[export import]spec/services/cache/clean_spec.rb (1)
15-18: Fix countries/cities cache key names to match implementation (_visited suffix required)Cache::Clean deletes "..._countries_visited" and "..._cities_visited", but the spec defines keys without the suffix; this will miss the intended keys and cause false results.
Apply:
- let(:user_1_countries_key) { "dawarich/user_#{user1.id}_countries" } - let(:user_2_countries_key) { "dawarich/user_#{user2.id}_countries" } - let(:user_1_cities_key) { "dawarich/user_#{user1.id}_cities" } - let(:user_2_cities_key) { "dawarich/user_#{user2.id}_cities" } + let(:user_1_countries_key) { "dawarich/user_#{user1.id}_countries_visited" } + let(:user_2_countries_key) { "dawarich/user_#{user2.id}_countries_visited" } + let(:user_1_cities_key) { "dawarich/user_#{user1.id}_cities_visited" } + let(:user_2_cities_key) { "dawarich/user_#{user2.id}_cities_visited" }app/models/stat.rb (2)
39-53: LGTM: robust expiration handling.Safely parses/guards nils and malformed timestamps; resolves the previous risk.
63-78: Validate/normalize expiration and support 'permanent'/'1w'; reject unknowns.Prevents silent nil expires_at and aligns UX with expectations; also reduces method length.
def enable_sharing!(expiration: '1h') - expires_at = case expiration - when '1h' then 1.hour.from_now - when '12h' then 12.hours.from_now - when '24h' then 24.hours.from_now - end + duration = { + '1h' => 1.hour, + '12h' => 12.hours, + '24h' => 24.hours, + '1w' => 1.week, + '1_week' => 1.week, + 'permanent' => nil + }[expiration] + raise ArgumentError, "Unknown expiration: #{expiration}" if duration.nil? && expiration != 'permanent' + expires_at = duration&.from_nowapp/controllers/stats_controller.rb (1)
19-25: Guard params and nil average; current code can miscompute/raise.Validate month ∈ 1..12 and avoid nil/1000. Matches prior feedback.
def month - @year = params[:year].to_i - @month = params[:month].to_i - @stat = current_user.stats.find_by(year: @year, month: @month) - @previous_stat = current_user.stats.find_by(year: @year, month: @month - 1) if @month > 1 - @average_distance_this_year = current_user.stats.where(year: @year).average(:distance).to_i / 1000 + @year = Integer(params[:year], exception: false) + @month = Integer(params[:month], exception: false) + return head :bad_request unless @year && @month && (1..12).cover?(@month) + + @stat = current_user.stats.find_by(year: @year, month: @month) + @previous_stat = current_user.stats.find_by(year: @year, month: @month - 1) if @month > 1 + avg_m = current_user.stats.where(year: @year).average(:distance) + @average_distance_this_year = avg_m.to_f / 1000.0 endapp/queries/hexagon_query.rb (3)
9-18: Coerce and validate inputs early (floats/ints/times).Prevents bad types and simplifies bind casting; aligns with security guidance.
def initialize(min_lon:, min_lat:, max_lon:, max_lat:, hex_size:, user_id: nil, start_date: nil, end_date: nil) - @min_lon = min_lon - @min_lat = min_lat - @max_lon = max_lon - @max_lat = max_lat - @hex_size = hex_size - @user_id = user_id - @start_date = start_date - @end_date = end_date + @min_lon = Float(min_lon) + @min_lat = Float(min_lat) + @max_lon = Float(max_lon) + @max_lat = Float(max_lat) + @hex_size = Float(hex_size) + @user_id = user_id.nil? ? nil : Integer(user_id) + @start_date = start_date&.to_s + @end_date = end_date&.to_s end
20-28: Bind handling is fragile; use typed binds or sanitize_sql_array.
exec_querywith$nplaceholders expects typed binds; raw values can fail under prepared statements.Minimal patch using typed attributes:
# replace binds (Array of raw values) with: connection = ActiveRecord::Base.connection make = ->(name, val, type) { ActiveRecord::Relation::QueryAttribute.new(name, val, type) } binds = [ make('min_lon', min_lon, ActiveRecord::Type::Float.new), make('min_lat', min_lat, ActiveRecord::Type::Float.new), make('max_lon', max_lon, ActiveRecord::Type::Float.new), make('max_lat', max_lat, ActiveRecord::Type::Float.new), make('hex_size', hex_size, ActiveRecord::Type::Float.new), make('limit', MAX_HEXAGONS_PER_REQUEST, ActiveRecord::Type::Integer.new) ] # append user_id/start/end similarly when present before calling exec_queryAlternatively, drop
$nand build withsanitize_sql_arrayto avoid manual index math.Also applies to: 98-117, 119-139
52-55: Fix PostGIS type mismatch in ST_Intersects (geography vs geometry).
lonlatis geography; comparing to a geometry bbox raises an error and bypasses geography indexes.- AND ST_Intersects( - lonlat, - (SELECT geom FROM bbox_geom)::geometry - ) + AND ST_Intersects( + lonlat, + (SELECT geom FROM bbox_geom)::geography + )app/controllers/api/v1/maps/hexagons_controller.rb (4)
28-30: Add composite index on points(user_id, timestamp) to support bounds/counts.Range scans and counts here will be slow without it.
31-49: Replace raw exec_query with AR aggregations; avoid$nbinds and to_i casts.Safer, simpler, and matches model-side refactor.
- if point_count.positive? - bounds_result = ActiveRecord::Base.connection.exec_query( - "SELECT MIN(latitude) as min_lat, MAX(latitude) as max_lat, - MIN(longitude) as min_lng, MAX(longitude) as max_lng - FROM points - WHERE user_id = $1 - AND timestamp BETWEEN $2 AND $3", - 'bounds_query', - [@target_user.id, @start_date.to_i, @end_date.to_i] - ).first - - render json: { - min_lat: bounds_result['min_lat'].to_f, - max_lat: bounds_result['max_lat'].to_f, - min_lng: bounds_result['min_lng'].to_f, - max_lng: bounds_result['max_lng'].to_f, - point_count: point_count - } + if point_count.positive? + min_lat, max_lat, min_lng, max_lng = points_relation.pick( + Arel.sql('MIN(latitude)'), + Arel.sql('MAX(latitude)'), + Arel.sql('MIN(longitude)'), + Arel.sql('MAX(longitude)') + ) + render json: { + min_lat: min_lat.to_f, + max_lat: max_lat.to_f, + min_lng: min_lng.to_f, + max_lng: max_lng.to_f, + point_count: point_count + }
91-95: Parse/validate authenticated date params in TZ; return 400 on invalid.Prevents bad ranges and downstream errors; aligns with earlier feedback.
def set_authenticated_context @target_user = current_api_user - @start_date = params[:start_date] - @end_date = params[:end_date] + @start_date = params[:start_date].present? ? Time.zone.parse(params[:start_date]) : nil + @end_date = params[:end_date].present? ? Time.zone.parse(params[:end_date]) : nil + unless @start_date && @end_date + render json: { error: 'Invalid or missing date range' }, status: :bad_request and return + end end
86-89: Use Time objects (not ISO strings) for dates in public sharing context.Strings later cause range/type mismatches against integer timestamps.
- @start_date = Date.new(@stat.year, @stat.month, 1).beginning_of_day.iso8601 - @end_date = Date.new(@stat.year, @stat.month, 1).end_of_month.end_of_day.iso8601 + @start_date = Time.zone.local(@stat.year, @stat.month, 1).beginning_of_day + @end_date = @start_date.end_of_month.end_of_dayapp/views/stats/_month.html.erb (3)
8-8: Avoid html_safe; build safe HTML via helpers.Removes XSS risk and satisfies Brakeman.
- <%= "#{icon month_icon(stat)} #{Date::MONTHNAMES[month]} #{year}".html_safe %> + <%= safe_join([icon(month_icon(stat)), "#{Date::MONTHNAMES[month]} #{year}"], ' ') %>
245-252: Guard against nil toponyms before iterating.Prevents exceptions when no location data exists.
- <% stat.toponyms.each do |country| %> - <% country['cities'].each do |city| %> - <div class="badge badge-outline"><%= city['city'] %></div> - <% end %> - <% end %> + <% if stat.toponyms.present? %> + <% stat.toponyms.each do |country| %> + <% country['cities'].each do |city| %> + <div class="badge badge-outline"><%= city['city'] %></div> + <% end %> + <% end %> + <% else %> + <span class="opacity-70">None</span> + <% end %>
56-59: Do not expose API keys in the DOM.Remove
data-api-key; rely on session/cookies or server-side proxy.- data-api-key="<%= current_user.api_key %>"app/services/maps/hexagon_grid.rb (1)
22-24: Add explicit date-range validation (reiterating prior feedback).Inputs can be malformed or reversed; validate format and ordering to fail fast at the model layer.
validate :validate_bbox_order validate :validate_area_size + validate :validate_date_range + + def validate_date_range + return unless start_date || end_date + s = start_date && parse_date_param(start_date) + e = end_date && parse_date_param(end_date) + errors.add(:base, 'start_date must be <= end_date') if s && e && s > e + rescue ArgumentError + errors.add(:base, 'Invalid start_date or end_date') + end + + def parse_date_param(v) + return Time.iso8601(v.to_s) unless v.is_a?(Numeric) + Time.at(v) + endapp/helpers/stats_helper.rb (2)
63-72: Nil/zero average can raise FloatDomainError.When
average_distance_this_yearis nil (e.g., first month), division by 0.0 leads to Infinity and.roundblows up.- return '' if average_distance_this_year&.zero? + return '' if average_distance_this_year.nil? || average_distance_this_year.to_f.zero? current_km = stat.distance / 1000.0 - difference = current_km - average_distance_this_year.to_f - percentage = ((difference / average_distance_this_year.to_f) * 100).round + avg = average_distance_this_year.to_f + difference = current_km - avg + percentage = ((difference / avg) * 100).round
10-47: Fix empty-year crash and nil-safe city extraction.
flatten!.uniq!can return nil and break;t['cities']may be nil. Also simplify and de-duplicate.- def countries_and_cities_stat_for_year(year, stats) - data = { countries: [], cities: [] } - - stats.select { _1.year == year }.each do - data[:countries] << _1.toponyms.flatten.map { |t| t['country'] }.uniq.compact - data[:cities] << _1.toponyms.flatten.flat_map { |t| t['cities'].map { |c| c['city'] } }.compact.uniq - end - - data[:cities].flatten!.uniq! - data[:countries].flatten!.uniq! - - grouped_by_country = {} - stats.select { _1.year == year }.each do |stat| - stat.toponyms.flatten.each do |toponym| - country = toponym['country'] - next if country.blank? - - grouped_by_country[country] ||= [] - - next if toponym['cities'].blank? - - toponym['cities'].each do |city_data| - city = city_data['city'] - grouped_by_country[country] << city if city.present? - end - end - end - - grouped_by_country.transform_values!(&:uniq) - - { - countries_count: data[:countries].count, - cities_count: data[:cities].count, - grouped_by_country: grouped_by_country.transform_values(&:sort).sort.to_h, - year: year, - modal_id: "countries_cities_modal_#{year}" - } - end + def countries_and_cities_stat_for_year(year, stats) + year_stats = stats.select { _1.year == year } + + countries = year_stats + .flat_map { |s| s.toponyms.flatten.map { |t| t['country'] } } + .compact.uniq + + cities = year_stats + .flat_map { |s| s.toponyms.flatten.flat_map { |t| t['cities']&.map { |c| c['city'] } || [] } } + .compact.uniq + + grouped_by_country = {} + year_stats.each do |stat| + stat.toponyms.flatten.each do |toponym| + country = toponym['country'] + next if country.blank? + grouped_by_country[country] ||= [] + city_names = toponym['cities']&.filter_map { |city_data| city_data['city'] } || [] + grouped_by_country[country].concat(city_names) + end + end + grouped_by_country.transform_values! { |arr| arr.compact.uniq } + + { + countries_count: countries.count, + cities_count: cities.count, + grouped_by_country: grouped_by_country.transform_values(&:sort).sort.to_h, + year: year, + modal_id: "countries_cities_modal_#{year}" + } + end
🧹 Nitpick comments (20)
Gemfile (4)
6-6: Align Ruby toolchain with Docker base image (bump to 3.4.5).Given the open review asking to move Dockerfile.dev to Ruby 3.4.5, please update
.ruby-versionaccordingly soruby File.read('.ruby-version')resolves to 3.4.5, and ensure Gemfile/CI use the same version. This avoids CVE drift and build mismatch.
12-12: Keep AWS SDK gems in lockstep to prevent resolution conflicts.You’re pinning aws-sdk-core (3.215.1), kms (1.96.0), and s3 (1.177.0). Recommend updating these as a set (same update window) and verifying the lock resolves cleanly; otherwise, consider relaxing the explicit core pin and letting service gems pull a compatible core.
32-32: Consider pinning rails_icons to a tested minor.Unpinned UI helper gems can ship breaking changes; add a pessimistic constraint once you’ve confirmed the version in Gemfile.lock.
43-43: Pin Sentry gems together and verify Rails 8 compatibility.Recommend pessimistically pinning
sentry-railsandsentry-rubyto the same tested minor to avoid API drift; double-check initializer compatibility with Rails 8.spec/jobs/users/mailer_sending_job_spec.rb (1)
110-118: Also assert no delivery attempt when user is missingStrengthen the test to ensure the mailer isn’t invoked at all and fail fast on setup.
- it 'does not raise an error' do - user.destroy + it 'does not raise an error or attempt delivery' do + user.destroy! + expect(UsersMailer).not_to receive(:with) expect do described_class.perform_now(user.id, 'welcome') end.not_to raise_error endapp/jobs/cache/preheating_job.rb (2)
14-18: Batch the three cache writes withwrite_multito cut Redis round-trips.One network call instead of three; same TTL.
Apply:
- Rails.cache.write( - "dawarich/user_#{user.id}_points_geocoded_stats", - StatsQuery.new(user).cached_points_geocoded_stats, - expires_in: 1.day - ) - - Rails.cache.write( - "dawarich/user_#{user.id}_countries_visited", - user.countries_visited_uncached, - expires_in: 1.day - ) - - Rails.cache.write( - "dawarich/user_#{user.id}_cities_visited", - user.cities_visited_uncached, - expires_in: 1.day - ) + Rails.cache.write_multi( + { + "dawarich/user_#{user.id}_points_geocoded_stats" => StatsQuery.new(user).cached_points_geocoded_stats, + "dawarich/user_#{user.id}_countries_visited" => user.countries_visited_uncached, + "dawarich/user_#{user.id}_cities_visited" => user.cities_visited_uncached + }, + expires_in: 1.day + )Also applies to: 20-24, 26-30
6-31: Make the job resilient per-user to avoid aborting the whole preheat.If one user errors, the remainder won’t cache. Prefer logging and continuing.
Apply:
def perform - User.find_each do |user| + User.find_each do |user| + begin Rails.cache.write( "dawarich/user_#{user.id}_years_tracked", user.years_tracked, expires_in: 1.day ) - - Rails.cache.write_multi( + + Rails.cache.write_multi( { "dawarich/user_#{user.id}_points_geocoded_stats" => StatsQuery.new(user).cached_points_geocoded_stats, "dawarich/user_#{user.id}_countries_visited" => user.countries_visited_uncached, "dawarich/user_#{user.id}_cities_visited" => user.cities_visited_uncached }, expires_in: 1.day ) + rescue => e + Rails.logger.warn("[Cache::PreheatingJob] user_id=#{user.id} error=#{e.class}: #{e.message}") + end end endapp/queries/stats_query.rb (3)
9-18: LGTM: cache key + TTL align with the preheater.Consider
race_condition_ttlto reduce stampedes under churn.Apply:
- cached_stats = Rails.cache.fetch("dawarich/user_#{user.id}_points_geocoded_stats", expires_in: 1.day) do + cached_stats = Rails.cache.fetch("dawarich/user_#{user.id}_points_geocoded_stats", expires_in: 1.day, race_condition_ttl: 10.seconds) do cached_points_geocoded_stats end
20-32: Prefer Postgres FILTER clauses for clarity and planner friendliness.More explicit than counting a nullable column + CASE.
Apply:
- <<~SQL.squish, - SELECT - COUNT(reverse_geocoded_at) as geocoded, - COUNT(CASE WHEN geodata = '{}'::jsonb THEN 1 END) as without_data - FROM points - WHERE user_id = ? - SQL + <<~SQL.squish, + SELECT + COUNT(*) FILTER (WHERE reverse_geocoded_at IS NOT NULL) AS geocoded, + COUNT(*) FILTER (WHERE geodata = '{}'::jsonb) AS without_data + FROM points + WHERE user_id = ? + SQL
20-32: Indexing advice to speed counts at scale.If needed, add partial indexes to back filtered counts and optionally split the query:
- DB migrations (CONCURRENTLY in production):
def up execute "CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_points_user_revgeo ON points (user_id) WHERE reverse_geocoded_at IS NOT NULL" execute "CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_points_user_geodata_empty ON points (user_id) WHERE geodata = '{}'::jsonb" end def down execute "DROP INDEX CONCURRENTLY IF EXISTS idx_points_user_revgeo" execute "DROP INDEX CONCURRENTLY IF EXISTS idx_points_user_geodata_empty" end
- Optional SQL rewrite to leverage them:
SELECT
(SELECT count() FROM points WHERE user_id = $1 AND reverse_geocoded_at IS NOT NULL) AS geocoded,
(SELECT count() FROM points WHERE user_id = $1 AND geodata = '{}'::jsonb) AS without_data;spec/services/cache/clean_spec.rb (2)
85-90: Make logging expectation less brittle (use spy style)Current expectations will fail if additional info logs are added. Spy style asserts presence without forbidding extra logs.
- it 'logs cache cleaning process' do - expect(Rails.logger).to receive(:info).with('Cleaning cache...') - expect(Rails.logger).to receive(:info).with('Cache cleaned') - - described_class.call - end + it 'logs cache cleaning process' do + allow(Rails.logger).to receive(:info).and_call_original + described_class.call + expect(Rails.logger).to have_received(:info).with('Cleaning cache...') + expect(Rails.logger).to have_received(:info).with('Cache cleaned') + end
92-111: Optional: strengthen the “user added during execution” specAlso assert user1/user2 caches were cleared and (optionally) create/assert user3’s countries/cities caches remain to exercise the new deletion routine.
expect { described_class.call }.not_to raise_error # The new user's cache should still exist since it wasn't processed expect(Rails.cache.exist?("dawarich/user_#{user3.id}_years_tracked")).to be true expect(Rails.cache.exist?("dawarich/user_#{user3.id}_points_geocoded_stats")).to be true + expect(Rails.cache.exist?("dawarich/user_#{user1.id}_years_tracked")).to be false + expect(Rails.cache.exist?("dawarich/user_#{user2.id}_years_tracked")).to be false + expect(Rails.cache.exist?("dawarich/user_#{user1.id}_points_geocoded_stats")).to be false + expect(Rails.cache.exist?("dawarich/user_#{user2.id}_points_geocoded_stats")).to be falseapp/queries/hexagon_query.rb (2)
141-147: Use Time.zone.parse for TZ consistency; return integer seconds directly.Avoids UTC/local mismatches and double conversions.
def parse_date_to_timestamp(date_string) - # Convert ISO date string to timestamp integer - Time.parse(date_string).to_i + # Convert ISO date string to timestamp integer in app timezone + Time.zone.parse(date_string).to_i rescue ArgumentError => e ExceptionReporter.call(e, "Invalid date format: #{date_string}") raise ArgumentError, "Invalid date format: #{date_string}" end
32-96: Optional: simplify bbox grid CTE and reduce method length.Extract SQL template to a frozen constant and build predicates separately; improves readability and satisfies RuboCop limits.
app/services/maps/hexagon_grid.rb (3)
34-35: Normalize start/end dates on ingest.Parse to canonical Time now to avoid type ambiguity downstream and keep metadata consistent.
- @start_date = params[:start_date] - @end_date = params[:end_date] + @start_date = params[:start_date] + @end_date = params[:end_date] + # Optionally normalize here (instead of only validating), keeping originals for echoing if needed: + # @start_date = params[:start_date].nil? ? nil : parse_date_param(params[:start_date]) + # @end_date = params[:end_date].nil? ? nil : parse_date_param(params[:end_date])
91-131: Splitformat_hexagonsto cut complexity and satisfy RuboCop.Extract row formatting and metadata building to dedicated helpers.
- def format_hexagons(result) - total_points = 0 - hexagons = result.map do |row| - ... - end - { - 'type' => 'FeatureCollection', - 'features' => hexagons, - 'metadata' => { - ... - } - } - end + def format_hexagons(result) + hexagons, total_points = result.each_with_object([[], 0]) do |row, acc| + feature = build_feature_from_row(row) + acc[0] << feature + acc[1] += feature[:properties][:point_count] + end + build_feature_collection(hexagons, total_points) + end + + def build_feature_from_row(row) + point_count = row['point_count'].to_i + { + type: 'Feature', + id: row['id'], + geometry: JSON.parse(row['geojson']), + properties: { + hex_id: row['id'], + hex_i: row['hex_i'], + hex_j: row['hex_j'], + hex_size: hex_size, + point_count: point_count, + earliest_point: format_point_time(row['earliest_point']), + latest_point: format_point_time(row['latest_point']) + } + } + end + + def build_feature_collection(hexagons, total_points) + { + 'type' => 'FeatureCollection', + 'features' => hexagons, + 'metadata' => { + 'bbox' => [min_lon, min_lat, max_lon, max_lat], + 'area_km2' => area_km2.round(2), + 'hex_size_m' => hex_size, + 'count' => hexagons.count, + 'total_points' => total_points, + 'user_id' => user_id, + 'date_range' => build_date_range_metadata + } + } + end
15-17: Remove or use viewport fields.
viewport_width/heightandviewport_valid?are unused. Either compute an adaptivehex_sizefrom viewport or drop them.Also applies to: 147-153
app/helpers/stats_helper.rb (3)
56-61: Consistent rounding scale for distances.Use two decimals for display to avoid losing information at small distances.
- value = Stat.convert_distance(stat.distance, distance_unit).round + value = Stat.convert_distance(stat.distance, distance_unit).round(2)
115-126: Optional: keep distance precision inpeak_dayto matchdistance_traveled.- distance_value = Stat.convert_distance(peak[1], distance_unit).round + distance_value = Stat.convert_distance(peak[1], distance_unit).round(2)
171-220: Collapse month mappings into constants to reduce cyclomatic complexity.Replace large case statements with lookup hashes (color, gradient, image) keyed by month.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Gemfile(2 hunks)app/controllers/api/v1/maps/hexagons_controller.rb(1 hunks)app/controllers/application_controller.rb(3 hunks)app/controllers/settings/users_controller.rb(5 hunks)app/controllers/stats_controller.rb(1 hunks)app/helpers/stats_helper.rb(1 hunks)app/jobs/cache/preheating_job.rb(1 hunks)app/models/stat.rb(2 hunks)app/models/user.rb(3 hunks)app/queries/hexagon_query.rb(1 hunks)app/queries/stats_query.rb(1 hunks)app/services/maps/hexagon_grid.rb(1 hunks)app/views/stats/_month.html.erb(1 hunks)config/routes.rb(2 hunks)db/migrate/20220325100310_devise_create_users.rb(1 hunks)spec/jobs/users/mailer_sending_job_spec.rb(1 hunks)spec/requests/settings/users_spec.rb(5 hunks)spec/services/cache/clean_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- db/migrate/20220325100310_devise_create_users.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- app/models/user.rb
- app/controllers/application_controller.rb
- config/routes.rb
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop with Rails extensions to lint Ruby code
Files:
spec/jobs/users/mailer_sending_job_spec.rbapp/services/maps/hexagon_grid.rbspec/requests/settings/users_spec.rbapp/controllers/settings/users_controller.rbapp/queries/stats_query.rbapp/controllers/api/v1/maps/hexagons_controller.rbapp/models/stat.rbspec/services/cache/clean_spec.rbapp/controllers/stats_controller.rbapp/helpers/stats_helper.rbapp/jobs/cache/preheating_job.rbapp/queries/hexagon_query.rb
app/{controllers,services}/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use Sidekiq for any potentially long-running operations instead of performing them inline
Files:
app/services/maps/hexagon_grid.rbapp/controllers/settings/users_controller.rbapp/controllers/api/v1/maps/hexagons_controller.rbapp/controllers/stats_controller.rb
spec/{models,system,requests,features}/**/*_spec.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Include both unit and integration tests for location-based features
Files:
spec/requests/settings/users_spec.rb
app/{models,queries}/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Leverage PostGIS (database-side) functions for geographic calculations instead of Ruby-based solutions
Files:
app/queries/stats_query.rbapp/models/stat.rbapp/queries/hexagon_query.rb
🧠 Learnings (3)
📚 Learning: 2025-09-04T20:15:35.655Z
Learnt from: CR
PR: Freika/dawarich#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T20:15:35.655Z
Learning: Applies to app/{models,queries}/**/*.rb : Leverage PostGIS (database-side) functions for geographic calculations instead of Ruby-based solutions
Applied to files:
app/queries/stats_query.rbapp/controllers/api/v1/maps/hexagons_controller.rbapp/queries/hexagon_query.rb
📚 Learning: 2025-09-04T20:15:35.655Z
Learnt from: CR
PR: Freika/dawarich#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T20:15:35.655Z
Learning: Applies to db/migrate/**/*.rb : Add appropriate database indexes for geographic queries to ensure performance
Applied to files:
app/controllers/api/v1/maps/hexagons_controller.rbapp/queries/hexagon_query.rb
📚 Learning: 2025-09-04T20:15:35.655Z
Learnt from: CR
PR: Freika/dawarich#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T20:15:35.655Z
Learning: Applies to app/models/point.rb : Use the `lonlat` column in the `points` table for geographic calculations
Applied to files:
app/controllers/api/v1/maps/hexagons_controller.rbapp/queries/hexagon_query.rb
🧬 Code graph analysis (8)
app/services/maps/hexagon_grid.rb (2)
app/queries/hexagon_query.rb (3)
attr_reader(3-148)initialize(9-18)call(20-28)app/javascript/controllers/public_stat_map_controller.js (1)
params(184-193)
app/controllers/settings/users_controller.rb (5)
app/controllers/api/v1/settings_controller.rb (1)
before_action(3-36)app/controllers/shared/stats_controller.rb (1)
before_action(3-54)app/controllers/trips_controller.rb (1)
before_action(3-70)app/controllers/visits_controller.rb (1)
before_action(3-38)app/controllers/settings/background_jobs_controller.rb (1)
before_action(3-26)
app/controllers/api/v1/maps/hexagons_controller.rb (4)
app/services/maps/hexagon_grid.rb (1)
call(38-42)app/javascript/controllers/public_stat_map_controller.js (1)
params(184-193)app/models/stat.rb (1)
public_accessible?(55-57)app/controllers/api_controller.rb (1)
current_api_user(29-31)
app/models/stat.rb (3)
app/controllers/stats_controller.rb (1)
month(19-25)app/services/stats/calculate_month.rb (1)
points(46-55)app/models/trip.rb (1)
points(20-22)
spec/services/cache/clean_spec.rb (1)
app/services/cache/clean.rb (2)
call(3-44)call(5-13)
app/helpers/stats_helper.rb (6)
app/models/concerns/distance_convertible.rb (1)
convert_distance(41-52)app/models/user.rb (2)
safe_settings(34-36)timezone(130-132)app/services/users/safe_settings.rb (1)
distance_unit(115-117)app/services/stats/calculate_month.rb (2)
toponyms(61-71)distance(57-59)app/services/countries_and_cities.rb (1)
new(3-54)app/controllers/stats_controller.rb (1)
month(19-25)
app/jobs/cache/preheating_job.rb (2)
app/queries/stats_query.rb (1)
cached_points_geocoded_stats(20-40)app/models/user.rb (2)
countries_visited_uncached(134-141)cities_visited_uncached(143-145)
app/queries/hexagon_query.rb (1)
app/services/maps/hexagon_grid.rb (2)
initialize(25-36)call(38-42)
🪛 ast-grep (0.38.6)
spec/jobs/users/mailer_sending_job_spec.rb
[warning] 114-114: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.perform_now(user.id, 'welcome')
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
app/jobs/cache/preheating_job.rb
[warning] 13-17: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: Rails.cache.write(
"dawarich/user_#{user.id}_points_geocoded_stats",
StatsQuery.new(user).cached_points_geocoded_stats,
expires_in: 1.day
)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 19-23: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: Rails.cache.write(
"dawarich/user_#{user.id}_countries_visited",
user.countries_visited_uncached,
expires_in: 1.day
)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 25-29: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: Rails.cache.write(
"dawarich/user_#{user.id}_cities_visited",
user.cities_visited_uncached,
expires_in: 1.day
)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🪛 RuboCop (1.76.1)
app/services/maps/hexagon_grid.rb
[convention] 3-153: Class has too many lines. [110/100]
(Metrics/ClassLength)
[convention] 25-36: Assignment Branch Condition size for initialize is too high. [<10, 17, 4> 20.12/17]
(Metrics/AbcSize)
[convention] 50-62: Assignment Branch Condition size for calculate_area_km2 is too high. [<5, 19, 0> 19.65/17]
(Metrics/AbcSize)
[convention] 91-131: Assignment Branch Condition size for format_hexagons is too high. [<7, 32, 3> 32.89/17]
(Metrics/AbcSize)
[convention] 91-131: Method has too many lines. [34/10]
(Metrics/MethodLength)
app/controllers/api/v1/maps/hexagons_controller.rb
[convention] 8-21: Method has too many lines. [11/10]
(Metrics/MethodLength)
[convention] 23-55: Assignment Branch Condition size for bounds is too high. [<3, 22, 5> 22.76/17]
(Metrics/AbcSize)
[convention] 23-55: Method has too many lines. [27/10]
(Metrics/MethodLength)
[convention] 77-89: Assignment Branch Condition size for set_public_sharing_context is too high. [<4, 17, 3> 17.72/17]
(Metrics/AbcSize)
app/models/stat.rb
[convention] 63-78: Method has too many lines. [13/10]
(Metrics/MethodLength)
[convention] 90-116: Assignment Branch Condition size for calculate_data_bounds is too high. [<5, 28, 1> 28.46/17]
(Metrics/AbcSize)
[convention] 90-116: Method has too many lines. [21/10]
(Metrics/MethodLength)
app/controllers/stats_controller.rb
[convention] 19-25: Assignment Branch Condition size for month is too high. [<5, 19, 2> 19.75/17]
(Metrics/AbcSize)
app/helpers/stats_helper.rb
[convention] 3-221: Module has too many lines. [167/100]
(Metrics/ModuleLength)
[convention] 10-47: Assignment Branch Condition size for countries_and_cities_stat_for_year is too high. [<11, 52, 14> 54.96/17]
(Metrics/AbcSize)
[convention] 10-47: Cyclomatic complexity for countries_and_cities_stat_for_year is too high. [13/7]
(Metrics/CyclomaticComplexity)
[convention] 10-47: Method has too many lines. [28/10]
(Metrics/MethodLength)
[convention] 10-47: Perceived complexity for countries_and_cities_stat_for_year is too high. [13/8]
(Metrics/PerceivedComplexity)
[convention] 115-126: Assignment Branch Condition size for peak_day is too high. [<5, 20, 2> 20.71/17]
(Metrics/AbcSize)
[convention] 128-160: Assignment Branch Condition size for quietest_week is too high. [<14, 24, 9> 29.21/17]
(Metrics/AbcSize)
[convention] 128-160: Cyclomatic complexity for quietest_week is too high. [9/7]
(Metrics/CyclomaticComplexity)
[convention] 128-160: Method has too many lines. [20/10]
(Metrics/MethodLength)
[convention] 128-160: Perceived complexity for quietest_week is too high. [9/8]
(Metrics/PerceivedComplexity)
[convention] 171-186: Cyclomatic complexity for month_color is too high. [13/7]
(Metrics/CyclomaticComplexity)
[convention] 171-186: Method has too many lines. [14/10]
(Metrics/MethodLength)
[convention] 188-203: Cyclomatic complexity for month_gradient_classes is too high. [13/7]
(Metrics/CyclomaticComplexity)
[convention] 188-203: Method has too many lines. [14/10]
(Metrics/MethodLength)
[convention] 205-220: Assignment Branch Condition size for month_bg_image is too high. [<0, 13, 12> 17.69/17]
(Metrics/AbcSize)
[convention] 205-220: Cyclomatic complexity for month_bg_image is too high. [13/7]
(Metrics/CyclomaticComplexity)
[convention] 205-220: Method has too many lines. [14/10]
(Metrics/MethodLength)
app/queries/hexagon_query.rb
[convention] 3-148: Class has too many lines. [122/100]
(Metrics/ClassLength)
[convention] 9-9: Avoid parameter lists longer than 5 parameters. [8/5]
(Metrics/ParameterLists)
[convention] 98-117: Method has too many lines. [12/10]
(Metrics/MethodLength)
[convention] 119-139: Method has too many lines. [15/10]
(Metrics/MethodLength)
🪛 Brakeman (7.0.2)
app/views/stats/_month.html.erb
[weak] 8-8: Unescaped model attribute
Type: Cross-Site Scripting
Confidence: Weak
More info: https://brakemanscanner.org/docs/warning_types/cross_site_scripting
(Cross-Site Scripting)
🔇 Additional comments (14)
Gemfile (2)
22-22: JWT pin looks good.Pinned to
~> 2.8as previously requested; reduces surprise major changes.
38-38: rqrcode constraint LGTM.
~> 3.0is a safe minor pin.spec/jobs/users/mailer_sending_job_spec.rb (2)
111-117: Updated behavior for deleted user: LGTMSwitching the expectation to “not raise” matches the new job behavior that rescues missing users.
114-114: Static analysis false positiveThe “hardcoded RSA passphrase” warning isn’t applicable here; this line just calls the job with an email type. Nothing to change.
app/jobs/cache/preheating_job.rb (1)
20-24: Confirm intended data source parity between countries and cities.
countries_visited_uncachedusespoints.without_raw_data, butcities_visited_uncacheddoes not. Verify this asymmetry is intentional.Would you like a quick script to scan for other call sites to ensure UI expectations match?
Also applies to: 26-30
app/queries/stats_query.rb (1)
20-32: No change required —geodatais non-null with default{}.schema.rb declares
t.jsonb "geodata", default: {}, null: false(migration db/migrate/20240712141303_add_geodata_to_points.rb adds it), so the COALESCE fallback is unnecessary.spec/requests/settings/users_spec.rb (4)
18-18: LGTM: unauthenticated users should be redirected to sign-in.
Matches Devise behavior and the new global auth flow.
103-105: LGTM: flash key switched to :alert for authorization failures.
Consistent with centralized Pundit rescue.
112-114: LGTM: flash key switched to :alert for authorization failures.
Consistent and clearer UX.
122-124: LGTM: flash key switched to :alert for authorization failures.
Aligned with app-wide behavior.spec/services/cache/clean_spec.rb (1)
30-64: Core cache-clean specs look solidGood coverage for control flag, version, years_tracked, and points_geocoded_stats deletions.
app/models/stat.rb (3)
10-10: LGTM: sharing UUID generation on create.Correctly ensures a UUID exists for new records.
35-38: LGTM: clear boolean gate for sharing.Straightforward and consistent with how you persist the flag.
80-88: LGTM: disable path clears sharing settings safely.Keeps UUID intact (works with your regenerate helper).
| redirect_to settings_users_url, notice: 'User was successfully updated.' | ||
| else | ||
| redirect_to settings_users_url, notice: 'User could not be updated.', status: :unprocessable_entity | ||
| redirect_to settings_users_url, notice: 'User could not be updated.', status: :unprocessable_content | ||
| end |
There was a problem hiding this comment.
Don’t redirect with 422; render :edit with flash.now.
Redirect + 422 breaks HTTP semantics and loses validation context.
Apply this diff:
- else
- redirect_to settings_users_url, notice: 'User could not be updated.', status: :unprocessable_content
+ else
+ flash.now[:alert] = 'User could not be updated.'
+ render :edit, status: :unprocessable_content📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| redirect_to settings_users_url, notice: 'User was successfully updated.' | |
| else | |
| redirect_to settings_users_url, notice: 'User could not be updated.', status: :unprocessable_entity | |
| redirect_to settings_users_url, notice: 'User could not be updated.', status: :unprocessable_content | |
| end | |
| redirect_to settings_users_url, notice: 'User was successfully updated.' | |
| else | |
| flash.now[:alert] = 'User could not be updated.' | |
| render :edit, status: :unprocessable_content | |
| end |
🤖 Prompt for AI Agents
In app/controllers/settings/users_controller.rb around lines 20 to 23, the
controller currently redirects with status :unprocessable_content on validation
failure which breaks HTTP semantics and loses form validation context; instead,
stop redirecting and render :edit with status :unprocessable_entity (422) and
use flash.now (e.g. flash.now[:alert] or flash.now[:notice]) to show the failure
message so the @user instance and its errors are preserved for the view. Ensure
you remove the redirect_to in the else branch, call render :edit, set status to
:unprocessable_entity, and set the flash via flash.now so validation messages
remain visible.
| if @user.save | ||
| redirect_to settings_users_url, notice: 'User was successfully created' | ||
| else | ||
| redirect_to settings_users_url, notice: 'User could not be created.', status: :unprocessable_entity | ||
| redirect_to settings_users_url, notice: 'User could not be created.', status: :unprocessable_content | ||
| end |
There was a problem hiding this comment.
Don’t redirect with 422 on create; render :new with flash.now.
Keeps errors on the form and correct status semantics.
Apply this diff:
- else
- redirect_to settings_users_url, notice: 'User could not be created.', status: :unprocessable_content
+ else
+ flash.now[:alert] = 'User could not be created.'
+ render :new, status: :unprocessable_contentCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/controllers/settings/users_controller.rb around lines 33 to 37, the
controller currently redirects on failed create which loses @user.errors and
misuses 422; change the else branch to render the new template instead of
redirecting, set the response status to :unprocessable_entity (422), and use
flash.now (e.g. flash.now[:alert] or flash.now[:notice]) to show the message so
the form re-renders with validation errors preserved.
| if @user.destroy | ||
| redirect_to settings_url, notice: 'User was successfully deleted.' | ||
| else | ||
| redirect_to settings_url, notice: 'User could not be deleted.', status: :unprocessable_entity | ||
| redirect_to settings_url, notice: 'User could not be deleted.', status: :unprocessable_content | ||
| end |
There was a problem hiding this comment.
Avoid 422 on redirect after failed destroy.
Use a normal redirect (303/302) or render an error page; 422 should accompany a rendered response.
Apply this diff:
- else
- redirect_to settings_url, notice: 'User could not be deleted.', status: :unprocessable_content
+ else
+ redirect_to settings_url, alert: 'User could not be deleted.'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if @user.destroy | |
| redirect_to settings_url, notice: 'User was successfully deleted.' | |
| else | |
| redirect_to settings_url, notice: 'User could not be deleted.', status: :unprocessable_entity | |
| redirect_to settings_url, notice: 'User could not be deleted.', status: :unprocessable_content | |
| end | |
| if @user.destroy | |
| redirect_to settings_url, notice: 'User was successfully deleted.' | |
| else | |
| redirect_to settings_url, alert: 'User could not be deleted.' | |
| end |
🤖 Prompt for AI Agents
In app/controllers/settings/users_controller.rb around lines 43 to 47, the
failure branch is issuing a redirect with status :unprocessable_content (422),
which is inappropriate for redirects; remove the status option from the
redirect_to so it uses the default redirect status (302) or explicitly set a
redirect status like :see_other (303) if needed, or alternatively render an
error view instead of redirecting if you intend to return a 422 response.
| def countries_and_cities_stat_for_month(stat) | ||
| countries = stat.toponyms.count { _1['country'] } | ||
| cities = stat.toponyms.sum { _1['cities'].count } | ||
|
|
||
| "#{countries} countries, #{cities} cities" | ||
| end |
There was a problem hiding this comment.
Guard nil cities in monthly aggregation.
Avoid calling .count on nil.
- countries = stat.toponyms.count { _1['country'] }
- cities = stat.toponyms.sum { _1['cities'].count }
+ countries = stat.toponyms.count { _1['country'].present? }
+ cities = stat.toponyms.sum { _1['cities']&.count.to_i }🤖 Prompt for AI Agents
In app/helpers/stats_helper.rb around lines 49 to 54, the monthly aggregation
calls `.count` on `stat.toponyms` entries' 'cities' without guarding against
nil; update the cities summation to avoid nil by using a safe navigation or
wrapping in an Array, e.g. replace the sum block with something that returns 0
for nil (for example use `_1['cities']&.count || 0` or
`Array(_1['cities']).count`) so the method never calls `.count` on nil.
| def countries_visited(stat) | ||
| stat.toponyms.count { _1['country'] } | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t count blank countries.
Empty strings are truthy; use present?.
- stat.toponyms.count { _1['country'] }
+ stat.toponyms.count { _1['country'].present? }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def countries_visited(stat) | |
| stat.toponyms.count { _1['country'] } | |
| end | |
| def countries_visited(stat) | |
| stat.toponyms.count { _1['country'].present? } | |
| end |
🤖 Prompt for AI Agents
In app/helpers/stats_helper.rb around lines 96 to 98, the countries_visited
method counts any non-nil country value but treats empty strings as truthy;
change the predicate to check presence (e.g. use _1['country'].present?) so
blank strings are excluded — update the block passed to count to call present?
on the country value.
| # Parse timestamps and format dates | ||
| earliest = row['earliest_point'] ? Time.zone.at(row['earliest_point'].to_f).iso8601 : nil | ||
| latest = row['latest_point'] ? Time.zone.at(row['latest_point'].to_f).iso8601 : nil | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Be robust to timestamp type when formatting dates.
If the adapter returns a String timestamp, to_f yields 2025.0-style values. Guard both numeric epoch and Time/String.
- earliest = row['earliest_point'] ? Time.zone.at(row['earliest_point'].to_f).iso8601 : nil
- latest = row['latest_point'] ? Time.zone.at(row['latest_point'].to_f).iso8601 : nil
+ earliest = format_point_time(row['earliest_point'])
+ latest = format_point_time(row['latest_point'])Add helper:
+ def format_point_time(v)
+ return nil if v.nil?
+ case v
+ when Numeric then Time.zone.at(v).iso8601
+ when Time then v.in_time_zone.iso8601
+ else
+ Time.zone.parse(v.to_s).iso8601
+ end
+ rescue ArgumentError
+ nil
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Parse timestamps and format dates | |
| earliest = row['earliest_point'] ? Time.zone.at(row['earliest_point'].to_f).iso8601 : nil | |
| latest = row['latest_point'] ? Time.zone.at(row['latest_point'].to_f).iso8601 : nil | |
| # Parse timestamps and format dates | |
| earliest = format_point_time(row['earliest_point']) | |
| latest = format_point_time(row['latest_point']) | |
| def format_point_time(v) | |
| return nil if v.nil? | |
| case v | |
| when Numeric then Time.zone.at(v).iso8601 | |
| when Time then v.in_time_zone.iso8601 | |
| else | |
| Time.zone.parse(v.to_s).iso8601 | |
| end | |
| rescue ArgumentError | |
| nil | |
| end |
🤖 Prompt for AI Agents
In app/services/maps/hexagon_grid.rb around lines 98 to 101, the timestamp
formatting assumes numeric epoch and uses to_f (which mis-parses string
timestamps); create a small helper (e.g. format_timestamp(value)) that accepts
nil, Numeric, String, or Time, returns nil for nil, returns an ISO8601 string
for valid times by: if value is a Time use Time.zone.local_to_utc(value).iso8601
or Time.zone.parse(value).iso8601 for String (rescue nil on parse failure), and
for Numeric use Time.zone.at(value.to_f).iso8601; then replace the two inline
expressions with calls to this helper for earliest and latest.
| def build_date_range_metadata | ||
| return nil unless start_date || end_date | ||
|
|
||
| { 'start_date' => start_date, 'end_date' => end_date } | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Emit normalized ISO8601 in date_range metadata.
Return consistent strings regardless of input type.
- { 'start_date' => start_date, 'end_date' => end_date }
+ {
+ 'start_date' => format_point_time(start_date),
+ 'end_date' => format_point_time(end_date)
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/services/maps/hexagon_grid.rb around lines 133 to 137, the
build_date_range_metadata method currently returns start_date and end_date as-is
which can be inconsistent; normalize and return ISO8601 strings. Update the
method to return nil when both are nil, and for each of start_date and end_date,
convert to an ISO8601 string (e.g. call iso8601 on Date/Time objects, or
parse/strftime to ISO8601 for strings) so the hash always contains normalized
ISO8601 string values for 'start_date' and 'end_date'.
| it 'renders a response with 422 status (i.e. to display the "new" template)' do | ||
| post settings_users_url, params: { user: invalid_attributes } | ||
|
|
||
| expect(response).to have_http_status(:unprocessable_entity) | ||
| expect(response).to have_http_status(:unprocessable_content) | ||
| end |
There was a problem hiding this comment.
Avoid 422 + redirect; assert non-redirect in spec.
Controller currently uses redirect_to ..., status: :unprocessable_content which is semantically wrong (422 responses shouldn’t redirect). Prefer rendering the form with 422. Strengthen the spec to ensure it’s not a redirect.
Apply this diff to the spec:
it 'renders a response with 422 status (i.e. to display the "new" template)' do
post settings_users_url, params: { user: invalid_attributes }
- expect(response).to have_http_status(:unprocessable_content)
+ expect(response).to have_http_status(:unprocessable_content)
+ expect(response).not_to be_redirect
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it 'renders a response with 422 status (i.e. to display the "new" template)' do | |
| post settings_users_url, params: { user: invalid_attributes } | |
| expect(response).to have_http_status(:unprocessable_entity) | |
| expect(response).to have_http_status(:unprocessable_content) | |
| end | |
| it 'renders a response with 422 status (i.e. to display the "new" template)' do | |
| post settings_users_url, params: { user: invalid_attributes } | |
| expect(response).to have_http_status(:unprocessable_content) | |
| expect(response).not_to be_redirect | |
| end |
🤖 Prompt for AI Agents
In spec/requests/settings/users_spec.rb around lines 64 to 68, the spec
currently only checks for a 422 status but doesn't assert that the response is
not a redirect; update the test to assert the response is not a redirect and
that the controller renders the form template with the correct 422 symbol.
Replace the single status check with assertions like: expect(response).not_to
be_redirect, expect(response).to render_template(:new), and expect(response).to
have_http_status(:unprocessable_entity) so the spec ensures rendering (not
redirecting) with the proper status.
| let(:user_1_countries_key) { "dawarich/user_#{user1.id}_countries" } | ||
| let(:user_2_countries_key) { "dawarich/user_#{user2.id}_countries" } | ||
| let(:user_1_cities_key) { "dawarich/user_#{user1.id}_cities" } | ||
| let(:user_2_cities_key) { "dawarich/user_#{user2.id}_cities" } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Quick self-check script to prevent regressions in key names
Run to ensure spec only uses the _visited variants and that the concurrency test remains valid.
Also applies to: 66-83, 92-111
🏁 Script executed:
#!/bin/bash
# Keys must end with _countries_visited / _cities_visited
rg -nP "user_#\\{\\w+\\.id\\}_(countries|cities)(?!_visited)" spec/services/cache/clean_spec.rb || echo "OK: no bare countries/cities keys"
# Sanity-check the concurrency stub shape (no and_yield chaining)
rg -n "allow\\(User\\)\\.to receive\\(:find_each\\).*and_yield" spec/services/cache/clean_spec.rb && echo "WARN: and_yield found; replace with a single block stub"Length of output: 263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="spec/services/cache/clean_spec.rb"
echo "== Bare _countries/_cities without _visited =="
awk '/_countries|_cities/ && !/_visited/ {print NR ":" $0}' "$file" || true
echo
echo "== Context around allow(User).to receive(:find_each) =="
rg -n -C3 "allow\\(User\\)\\.to receive\\(:find_each\\)" "$file" || true
echo
echo "== Lines with and_yield =="
rg -n -C2 "and_yield" "$file" || trueLength of output: 902
Use _visited cache keys and replace chained and_yield in clean_spec.rb
- Replace bare keys "dawarich/user_#{user.id}countries" / "dawarich/user#{user.id}_cities" with "..._countries_visited" / "..._cities_visited" (examples: spec/services/cache/clean_spec.rb lines 15–18; scan ranges 66–83 and 92–111).
- Replace the chained stub on line 96:
allow(User).to receive(:find_each).and_yield(user1).and_yield(user2) do |&block|
with a single-block stub that yields each user, e.g.
allow(User).to receive(:find_each) { |&blk| [user1, user2].each(&blk) }.
🤖 Prompt for AI Agents
In spec/services/cache/clean_spec.rb around lines 15–18 (and scan ranges 66–83
and 92–111), the tests use bare cache keys and a chained and_yield stub; update
the keys to include the "_visited" suffix so the keys become
"..._countries_visited" and "..._cities_visited" for both users, and replace the
chained allow(User).to receive(:find_each).and_yield(user1).and_yield(user2)
stub (around line 96) with a single-block stub that yields each user, e.g.
implement allow(User).to receive(:find_each) { |&blk| [user1, user2].each(&blk)
} so the test yields users via the block rather than using chained and_yield.
| allow(User).to receive(:find_each).and_yield(user1).and_yield(user2) do |&block| | ||
| # Create a new user while iterating - this should not cause errors | ||
| user3 = create(:user) | ||
| Rails.cache.write("dawarich/user_#{user3.id}_years_tracked", { 2023 => ['May'] }) | ||
| Rails.cache.write("dawarich/user_#{user3.id}_points_geocoded_stats", { geocoded: 1, without_data: 0 }) | ||
|
|
||
| # Continue with the original block | ||
| [user1, user2].each(&block) | ||
| end |
There was a problem hiding this comment.
RSpec stub for find_each is incorrect (and_yield + block won’t execute your setup)
Chaining and_yield with a custom block means the block won’t run; user3 stays nil and the test can error. Provide a single block stub that performs the setup and yields user1/user2.
- allow(User).to receive(:find_each).and_yield(user1).and_yield(user2) do |&block|
- # Create a new user while iterating - this should not cause errors
- user3 = create(:user)
- Rails.cache.write("dawarich/user_#{user3.id}_years_tracked", { 2023 => ['May'] })
- Rails.cache.write("dawarich/user_#{user3.id}_points_geocoded_stats", { geocoded: 1, without_data: 0 })
-
- # Continue with the original block
- [user1, user2].each(&block)
- end
+ allow(User).to receive(:find_each) do |&block|
+ # Create a new user during iteration; it should not be processed in this run
+ user3 = create(:user)
+ Rails.cache.write("dawarich/user_#{user3.id}_years_tracked", { 2023 => ['May'] })
+ Rails.cache.write("dawarich/user_#{user3.id}_points_geocoded_stats", { geocoded: 1, without_data: 0 })
+ [user1, user2].each(&block)
+ end🤖 Prompt for AI Agents
In spec/services/cache/clean_spec.rb around lines 96 to 104, the stub uses
and_yield chained with a custom block so the custom setup block never runs and
user3 is not created; replace the chained and_yield approach with a single stub
block: use allow(User).to receive(:find_each) do |&block| ... end, create user3
and write its cache entries inside that block, then yield user1 and user2 by
calling the original block (e.g. [user1, user2].each(&block)) so the setup
executes and the test yields the intended users.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
app/controllers/api/v1/maps/hexagons_controller.rb (4)
56-58: Add composite index on points(user_id, timestamp) to keep this query fast.Range scans by user and timestamp will need a composite index.
Migration (separate file):
class AddUserTimestampIndexToPoints < ActiveRecord::Migration[7.1] def change add_index :points, [:user_id, :timestamp] end endRun to verify presence:
#!/bin/bash set -euo pipefail fd -a schema.rb db | xargs -I{} rg -nP '\bcreate_table\s+"points"|add_index.+points' {} -C2 || true rg -nP 'add_index\s+:points,\s*\[:user_id,\s*:timestamp\]' db/migrate db/schema.rb -C2 || true
28-55: Replace ad-hoc timestamp casting and raw SQL with AR aggregates; use Time.zone.Current code risks type mismatches (integers vs timestamptz), Postgres-specific binds, and timezone/DST bugs. Use parsed Time.zone objects and AR aggregations on the scoped relation.
Apply:
- # Convert dates to timestamps (handle both string and timestamp formats) - start_timestamp = case @start_date - when String - # Check if it's a numeric string (timestamp) or date string - if @start_date.match?(/^\d+$/) - @start_date.to_i - else - Time.parse(@start_date).to_i - end - when Integer - @start_date - else - @start_date.to_i - end - end_timestamp = case @end_date - when String - # Check if it's a numeric string (timestamp) or date string - if @end_date.match?(/^\d+$/) - @end_date.to_i - else - Time.parse(@end_date).to_i - end - when Integer - @end_date - else - @end_date.to_i - end - - points_relation = @target_user.points.where(timestamp: start_timestamp..end_timestamp) + # Dates are parsed upstream to Time.zone; rely on AR for proper binding + points_relation = @target_user.points.where(timestamp: @start_date..@end_date) @@ - if point_count.positive? - bounds_result = ActiveRecord::Base.connection.exec_query( - "SELECT MIN(latitude) as min_lat, MAX(latitude) as max_lat, - MIN(longitude) as min_lng, MAX(longitude) as max_lng - FROM points - WHERE user_id = $1 - AND timestamp BETWEEN $2 AND $3", - 'bounds_query', - [@target_user.id, start_timestamp, end_timestamp] - ).first - - render json: { - min_lat: bounds_result['min_lat'].to_f, - max_lat: bounds_result['max_lat'].to_f, - min_lng: bounds_result['min_lng'].to_f, - max_lng: bounds_result['max_lng'].to_f, - point_count: point_count - } + if point_count.positive? + min_lat, max_lat, min_lng, max_lng = + points_relation.pick( + Arel.sql('MIN(latitude)'), + Arel.sql('MAX(latitude)'), + Arel.sql('MIN(longitude)'), + Arel.sql('MAX(longitude)') + ) + + render json: { + min_lat: min_lat.to_f, + max_lat: max_lat.to_f, + min_lng: min_lng.to_f, + max_lng: max_lng.to_f, + point_count: point_count + }Also applies to: 56-58, 60-76
119-123: Parse and validate incoming date params; support start_at/end_at; enforce ordering.Avoid passing raw strings; normalize via Time.zone, accept alt names, and validate.
def set_authenticated_context @target_user = current_api_user - @start_date = params[:start_date] - @end_date = params[:end_date] + @start_date = parse_time_param(params[:start_date] || params[:start_at]) + @end_date = parse_time_param(params[:end_date] || params[:end_at]) + unless @start_date && @end_date && @start_date <= @end_date + render json: { error: 'Invalid or missing date range' }, status: :bad_request and return + end endAlso add this helper within the private section (see placement in next comment).
125-129: Add parse_time_param helper (private).Keeps parsing consistent and resilient.
def handle_service_error render json: { error: 'Failed to generate hexagon grid' }, status: :internal_server_error end + + def parse_time_param(val) + return nil if val.blank? + Time.zone.parse(val) + rescue ArgumentError, TypeError + nil + end
🧹 Nitpick comments (3)
app/controllers/api/v1/maps/hexagons_controller.rb (3)
59-76: Optional: Use PostGIS ST_Extent on lonlat for bounds (faster, index-friendly).If points.lonlat exists, prefer database-side geometry ops.
Example:
min_lon, min_lat, max_lon, max_lat = points_relation.pick( Arel.sql('ST_XMin(ST_Extent(lonlat))'), Arel.sql('ST_YMin(ST_Extent(lonlat))'), Arel.sql('ST_XMax(ST_Extent(lonlat))'), Arel.sql('ST_YMax(ST_Extent(lonlat))') )
19-21: Log unexpected errors before generic 500.Capturing context helps ops/debugging without leaking details to clients.
- rescue StandardError => _e - handle_service_error + rescue StandardError => e + Rails.logger.error("Hexagon service failure: #{e.class}: #{e.message}\n#{e.backtrace&.first(5)&.join("\n")}") + handle_service_error
23-83: Extract and slim down bounds to satisfy RuboCop metrics.Split parsing, relation build, and rendering into helpers to reduce ABC/cyclomatic complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/v1/maps/hexagons_controller.rb(1 hunks)app/services/cache/clean.rb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/services/cache/clean.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop with Rails extensions to lint Ruby code
Files:
app/controllers/api/v1/maps/hexagons_controller.rb
app/{controllers,services}/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use Sidekiq for any potentially long-running operations instead of performing them inline
Files:
app/controllers/api/v1/maps/hexagons_controller.rb
🧠 Learnings (3)
📚 Learning: 2025-09-04T20:15:35.655Z
Learnt from: CR
PR: Freika/dawarich#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T20:15:35.655Z
Learning: Applies to db/migrate/**/*.rb : Add appropriate database indexes for geographic queries to ensure performance
Applied to files:
app/controllers/api/v1/maps/hexagons_controller.rb
📚 Learning: 2025-09-04T20:15:35.655Z
Learnt from: CR
PR: Freika/dawarich#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T20:15:35.655Z
Learning: Applies to app/{models,queries}/**/*.rb : Leverage PostGIS (database-side) functions for geographic calculations instead of Ruby-based solutions
Applied to files:
app/controllers/api/v1/maps/hexagons_controller.rb
📚 Learning: 2025-09-04T20:15:35.655Z
Learnt from: CR
PR: Freika/dawarich#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T20:15:35.655Z
Learning: Applies to app/models/point.rb : Use the `lonlat` column in the `points` table for geographic calculations
Applied to files:
app/controllers/api/v1/maps/hexagons_controller.rb
🧬 Code graph analysis (1)
app/controllers/api/v1/maps/hexagons_controller.rb (4)
app/services/maps/hexagon_grid.rb (1)
call(38-42)app/javascript/controllers/public_stat_map_controller.js (1)
params(184-193)app/models/stat.rb (1)
public_accessible?(55-57)app/controllers/api_controller.rb (1)
current_api_user(29-31)
🪛 RuboCop (1.76.1)
app/controllers/api/v1/maps/hexagons_controller.rb
[convention] 3-143: Class has too many lines. [114/100]
(Metrics/ClassLength)
[convention] 8-21: Method has too many lines. [11/10]
(Metrics/MethodLength)
[convention] 23-83: Assignment Branch Condition size for bounds is too high. [<5, 30, 13> 33.08/17]
(Metrics/AbcSize)
[convention] 23-83: Cyclomatic complexity for bounds is too high. [11/7]
(Metrics/CyclomaticComplexity)
[convention] 23-83: Method has too many lines. [51/10]
(Metrics/MethodLength)
[convention] 23-83: Perceived complexity for bounds is too high. [12/8]
(Metrics/PerceivedComplexity)
[convention] 105-117: Assignment Branch Condition size for set_public_sharing_context is too high. [<4, 17, 3> 17.72/17]
(Metrics/AbcSize)
🔇 Additional comments (2)
app/controllers/api/v1/maps/hexagons_controller.rb (2)
133-142: LGTM: bbox param validation is clear and client-friendly.Error message enumerates missing fields; good.
8-21: LGTM: index action structure and error mapping.Service encapsulation and specific error-to-status mapping look good.
| @start_date = Date.new(@stat.year, @stat.month, 1).beginning_of_day.iso8601 | ||
| @end_date = Date.new(@stat.year, @stat.month, 1).end_of_month.end_of_day.iso8601 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Time.zone for public sharing range; don’t convert to ISO8601 strings.
Keep Time objects for AR bindings and timezone correctness.
- @start_date = Date.new(@stat.year, @stat.month, 1).beginning_of_day.iso8601
- @end_date = Date.new(@stat.year, @stat.month, 1).end_of_month.end_of_day.iso8601
+ period_start = Time.zone.local(@stat.year, @stat.month, 1).beginning_of_day
+ @start_date = period_start
+ @end_date = period_start.end_of_month.end_of_day📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @start_date = Date.new(@stat.year, @stat.month, 1).beginning_of_day.iso8601 | |
| @end_date = Date.new(@stat.year, @stat.month, 1).end_of_month.end_of_day.iso8601 | |
| period_start = Time.zone.local(@stat.year, @stat.month, 1).beginning_of_day | |
| @start_date = period_start | |
| @end_date = period_start.end_of_month.end_of_day |
🤖 Prompt for AI Agents
In app/controllers/api/v1/maps/hexagons_controller.rb around lines 115-116, the
controller currently builds start/end dates as Date objects converted to ISO8601
strings; change them to timezone-aware Time objects using Time.zone (e.g.
Time.zone.local(@stat.year, @stat.month, 1).beginning_of_day and
Time.zone.local(@stat.year, @stat.month, 1).end_of_month.end_of_day) and remove
the iso8601 conversion so ActiveRecord bindings receive Time objects with
correct zone.
Summary by CodeRabbit