Open
Conversation
satish2025-crew
added
enhancement
Contributor
📝 WalkthroughWalkthroughAdds a new Mongoid model Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/models/cron_job_run.rb (1)
7-15: Add validations for the core tracking fields.Right now callers can persist blank
job_name, nilstarted_at, or arbitrarystatusvalues. That will make these records much less reliable for the indexed lookups and status scopes this model is introducing.💡 Suggested change
field :jobs_succeeded, type: Integer, default: 0 field :jobs_failed, type: Integer, default: 0 field :metadata, type: Hash, default: {} + + VALID_STATUSES = %i[running failed succeeded].freeze + + validates :job_name, :started_at, :status, presence: true + validates :status, inclusion: { in: VALID_STATUSES }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/cron_job_run.rb` around lines 7 - 15, CronJobRun allows blank or invalid core fields; add ActiveModel validations on the model: validate presence (and not blank) of job_name and started_at, validate presence of status plus inclusion in a defined set (e.g. define STATUS_VALUES constant and use validates :status, inclusion: { in: STATUS_VALUES }), and add numeric/non-negative checks for jobs_processed, jobs_succeeded, and jobs_failed (e.g. validates_numericality_of ..., greater_than_or_equal_to: 0). Update the CronJobRun model to include these validations so indexed lookups and status scopes only store valid records.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/cron_job_run.rb`:
- Line 3: Add a top-level documentation comment for the CronJobRun class (class
CronJobRun) describing its responsibility and usage, or suppress the
Style/Documentation cop for this class by adding the appropriate RuboCop
directive above the class (e.g., # rubocop:disable Style/Documentation) if
documentation is intentionally omitted; update only the class declaration area
so CI no longer fails on the top-level documentation rule.
- Around line 28-41: The record method currently creates a CronJobRun and yields
it but never finalizes successful runs; change it so that after yielding
(whether the block raises or not) the record is updated with ended_at and the
appropriate status: on normal completion set status to :succeeded and ended_at
to DateTime.now, and on exception (rescue StandardError => e) keep the existing
behavior that sets status :failed, error_message and ended_at; implement this by
wrapping the yield in begin...rescue...ensure or using an ensure block that
updates run when present, referencing the existing class method
CronJobRun.record, the local run variable, and the create!/yield flow so callers
no longer must manually mark success.
- Line 26: Remove the redundant .freeze on the JOB_RETRY_FAILED_VLP constant in
the CronJobRun class: because the file has # frozen_string_literal: true, change
the constant declaration from JOB_RETRY_FAILED_VLP =
'retry_failed_vlp_jobs'.freeze to simply JOB_RETRY_FAILED_VLP =
'retry_failed_vlp_jobs' (update the declaration where the constant is defined).
In `@spec/models/cron_job_run_spec.rb`:
- Line 121: The example block passed to
described_class.record(CronJobRun::JOB_RETRY_FAILED_VLP) declares an unused
block parameter `run`, causing Lint/UnusedBlockArgument; remove the unused
parameter by changing the block from `do |run| ... end` to `do ... end` (i.e.,
drop `|run|`) wherever this invocation appears so the test no longer defines an
unused variable.
---
Nitpick comments:
In `@app/models/cron_job_run.rb`:
- Around line 7-15: CronJobRun allows blank or invalid core fields; add
ActiveModel validations on the model: validate presence (and not blank) of
job_name and started_at, validate presence of status plus inclusion in a defined
set (e.g. define STATUS_VALUES constant and use validates :status, inclusion: {
in: STATUS_VALUES }), and add numeric/non-negative checks for jobs_processed,
jobs_succeeded, and jobs_failed (e.g. validates_numericality_of ...,
greater_than_or_equal_to: 0). Update the CronJobRun model to include these
validations so indexed lookups and status scopes only store valid records.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a35b952f-267c-4da2-a7ba-4538060ffc55
📒 Files selected for processing (2)
app/models/cron_job_run.rbspec/models/cron_job_run_spec.rb
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/models/cron_job_run.rb (2)
12-12: Consider validatingstatusagainst allowed values.Without validation, invalid status symbols can be persisted, potentially breaking reporting and scope queries.
💡 Suggested addition
field :status, type: Symbol # :running, :succeeded, or :failed field :error_message, type: String # Set when status is :failed + STATUSES = %i[running succeeded failed].freeze + validates :status, inclusion: { in: STATUSES }, allow_nil: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/cron_job_run.rb` at line 12, Add a strict validation for the status field in the CronJobRun model: define a constant (e.g., STATUSES = [:running, :succeeded, :failed]) and add validates :status, inclusion: { in: STATUSES } so only those symbols can be persisted; update any code that sets or queries status to use CronJobRun::STATUSES (or the constant values) to avoid hardcoded strings/symbol mismatches.
9-13: Consider adding an index onjob_nameandstarted_at.Queries will frequently filter by
job_nameand sort/filter bystarted_at. Without indexes, performance will degrade as the collection grows.💡 Suggested addition
field :error_message, type: String # Set when status is :failed + index({ job_name: 1, started_at: -1 }) + index({ started_at: -1 }) + scope :recent, -> { order(started_at: :desc) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/cron_job_run.rb` around lines 9 - 13, Add database indexes to the CronJobRun model to speed queries that filter by job_name and sort/filter by started_at: add a single-field index on job_name and a compound index on {job_name, started_at} (started_at descending if you usually sort newest-first) in the model definition so queries against the job_name and started_at fields (field names: job_name, started_at; model: CronJobRun) use indexed lookups and avoid full collection scans.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/models/cron_job_run.rb`:
- Line 12: Add a strict validation for the status field in the CronJobRun model:
define a constant (e.g., STATUSES = [:running, :succeeded, :failed]) and add
validates :status, inclusion: { in: STATUSES } so only those symbols can be
persisted; update any code that sets or queries status to use
CronJobRun::STATUSES (or the constant values) to avoid hardcoded strings/symbol
mismatches.
- Around line 9-13: Add database indexes to the CronJobRun model to speed
queries that filter by job_name and sort/filter by started_at: add a
single-field index on job_name and a compound index on {job_name, started_at}
(started_at descending if you usually sort newest-first) in the model definition
so queries against the job_name and started_at fields (field names: job_name,
started_at; model: CronJobRun) use indexed lookups and avoid full collection
scans.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a5bf417-ddfd-44f2-99af-1e5bc288b5c3
📒 Files selected for processing (2)
app/models/cron_job_run.rbspec/models/cron_job_run_spec.rb
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/models/cron_job_run_spec.rb (1)
96-100: Consider using the record's actual timestamp for more robust date comparison.The
target_dateis computed independently from whenrun2was created. If the test crosses a midnight boundary (e.g., slow CI run), the dates could theoretically differ.💡 More robust approach
describe '.on_date' do it 'returns runs started on the given date' do - target_date = 23.hours.ago.to_date + target_date = run2.started_at.to_date expect(described_class.on_date(target_date).to_a).to contain_exactly(run2) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/cron_job_run_spec.rb` around lines 96 - 100, The test computes target_date independently which can diverge from run2's actual timestamp; change target_date to derive from the fixture's timestamp (e.g. use run2.started_at.to_date or run2.created_at.to_date) so the expectation calls described_class.on_date(target_date) use the record's real date, ensuring the spec is robust across midnight boundaries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/models/cron_job_run_spec.rb`:
- Around line 96-100: The test computes target_date independently which can
diverge from run2's actual timestamp; change target_date to derive from the
fixture's timestamp (e.g. use run2.started_at.to_date or
run2.created_at.to_date) so the expectation calls
described_class.on_date(target_date) use the record's real date, ensuring the
spec is robust across midnight boundaries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30e9097f-4b53-4e23-8b90-e6d726e2e7a9
📒 Files selected for processing (1)
spec/models/cron_job_run_spec.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://app.clickup.com/t/868hqn90h
Summary by CodeRabbit