chore: store release schedule without timezone #878
Conversation
Changed kickoff_at (datetime) to kickoff_time (time) to make release scheduling based on time of day rather than specific datetime. The scheduled_at for ScheduledRelease is still stored in UTC but is now calculated by combining kickoff_time with the app's configured timezone. This makes scheduling more intuitive - trains kick off at the same time of day in the app's timezone regardless of DST changes or other timezone complications. Changes: - Migration: Change kickoff_at column from datetime to time - Model: Update next_run_at logic to be timezone-aware - Model: Update validations to work with time instead of datetime - Controller: Update params and config methods - Views: Change datetime field to time field - Tests: Update all specs to work with new time-based scheduling - Helper: Update release_schedule display to show timezone BREAKING CHANGE: Existing trains with kickoff_at will need to run the migration to convert to kickoff_time
WalkthroughThe PR renames Train.kickoff_at (datetime) to kickoff_time (time-of-day) and updates models, controllers, helpers, views, fixtures, and tests to use the new attribute. Train scheduling logic was rewritten to compute timezone-aware kickoff datetimes using the train's app timezone (new kickoff_datetime method) and to derive next_run_at from kickoff_time and scheduled_releases. A reversible migration converts existing kickoff_at datetimes to kickoff_time (time component in app timezone) and removes kickoff_at. Form inputs, params, anonymization whitelist, and test factories/specs were adjusted accordingly. Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI / Controller
participant Train as Train model
participant TZ as App Timezone
participant SR as ScheduledRelease store
rect rgba(120,160,200,0.08)
UI->>Train: create/update with kickoff_time (HH:MM:SS)
Train->>TZ: kickoff_datetime(reference_time)
TZ-->>Train: timezone-aware datetime (kickoff datetime)
end
rect rgba(120,200,160,0.08)
Note right of Train: next_run_at calculation
Train->>SR: query scheduled_releases (ordered)
SR-->>Train: last_scheduled / releases list
Train->>Train: compute base_time (last_run_at or kickoff_datetime)
Train->>Train: add repeat_duration multiples -> next_run_at
Train-->>UI: return next_run_at (UTC)
end
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)app/models/train.rb (2)
🪛 GitHub Check: lintapp/models/train.rb[failure] 248-248: 🔇 Additional comments (1)
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 |
kitallis
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/trains_controller.rb (1)
199-205: Potential crash + param mix-up inparsed_train_update_paramswhenversion_bump_file_pathsis nil
parsed_train_update_paramscurrently does:.merge(version_bump_file_paths: train_params.fetch(:version_bump_file_paths, "").safe_csv_parse(coerce_string: true))Two issues:
- If
:version_bump_file_pathsis present but explicitlynil,fetchreturnsniland.safe_csv_parsewill raiseNoMethodError.- Using
train_paramsin the update path is confusing and may diverge fromtrain_update_params's strong-parameter rules over time.You can mirror the create path and fix the nil case with:
- .merge(version_bump_file_paths: train_params.fetch(:version_bump_file_paths, "").safe_csv_parse(coerce_string: true)) + .merge( + version_bump_file_paths: train_update_params + .fetch(:version_bump_file_paths, "") + &.safe_csv_parse(coerce_string: true) + )This keeps behavior consistent between create/update and avoids crashes when the field is present but blank.
🧹 Nitpick comments (4)
app/helpers/trains_helper.rb (1)
2-8: Consider I18n + small guard aroundkickoff_timeformattingThe new
"Runs every #{duration} at #{time} (#{timezone})"message is clear and surfaces the timezone, which matches the PR intent. To make this a bit more robust and localizable you might:
- Use I18n/l helpers for the time instead of hardcoded
strftime, and- Optionally guard against a missing
kickoff_timeto avoidNoMethodErroron misconfigured records.For example:
- time = train.kickoff_time.strftime("%I:%M%p") + time = train.kickoff_time && l(train.kickoff_time, format: :short)Not strictly required, but improves resilience and localization.
spec/factories/trains.rb (1)
50-54: Align factory time arithmetic with Rails style (and lint)
kickoff_time { (Time.current + 2.hours).strftime("%H:%M:%S") }works but triggersRails/DurationArithmeticand is slightly less idiomatic.You can keep the same behavior and satisfy the cop with:
- kickoff_time { (Time.current + 2.hours).strftime("%H:%M:%S") } + kickoff_time { 2.hours.from_now.strftime("%H:%M:%S") }This also matches the previous
kickoff_at { 2.hours.from_now }style.spec/jobs/schedule_train_releases_job_spec.rb (1)
7-17: Timezone-aware setup looks good; consider tightening time arithmetic for clarityThe updated specs correctly:
- Use the app’s timezone to construct
first_scheduled, then store it as UTC, and- Assert that the next scheduled release is
first_scheduled + train.repeat_duration, which exercises the newnext_run_atlogic.A couple of small polish points for robustness and readability:
- Prefer idiomatic duration helpers over
Time.current + 2.hours:- future_time = (Time.current + 2.hours).strftime("%H:%M:%S") + future_time = 2.hours.from_now.strftime("%H:%M:%S")
- Avoid calling
Time.currenttwice with potentially different zones when buildingfuture_scheduled:- future_scheduled = Time.current.in_time_zone(tz).change(hour: Time.current.hour + 2).utc + now_in_tz = Time.current.in_time_zone(tz) + future_scheduled = now_in_tz.change(hour: now_in_tz.hour + 2).utcOptionally, you could also
travel_toa fixed base time in this example for extra determinism, but it’s not strictly necessary given the current assertions.Also applies to: 30-37
spec/models/train_spec.rb (1)
200-206: Tidy up time arithmetic in update-spec to satisfy lint and match Rails idiomsThe behavior in this example is correct, but
future_time = (Time.current + 2.hours).strftime("%H:%M:%S")is flagged byRails/DurationArithmeticand is slightly non-idiomatic.You can keep the same semantics and appease the linter by using:
- future_time = (Time.current + 2.hours).strftime("%H:%M:%S") + future_time = 2.hours.from_now.strftime("%H:%M:%S")The subsequent expectations comparing
scheduled_atin the app timezone withtrain.kickoff_timeremain valid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/components/release_list_component.rb(1 hunks)app/controllers/trains_controller.rb(3 hunks)app/helpers/trains_helper.rb(1 hunks)app/models/train.rb(5 hunks)app/views/trains/_release_schedule_form.html.erb(1 hunks)db/data/20240305124235_connect_scheduled_releases_with_release.rb(1 hunks)db/migrate/20251118165553_change_kickoff_at_to_kickoff_time.rb(1 hunks)lib/tasks/anonymize.rake(1 hunks)spec/components/scheduled_train_component_spec.rb(8 hunks)spec/factories/trains.rb(1 hunks)spec/jobs/schedule_train_releases_job_spec.rb(2 hunks)spec/models/train_spec.rb(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
spec/factories/trains.rb (1)
app/javascript/controllers/countdown_controller.js (1)
hours(33-33)
spec/jobs/schedule_train_releases_job_spec.rb (3)
app/components/event_timeline_component.rb (1)
tz(73-73)db/migrate/20230803115802_add_scheduled_releases_attempts_for_train.rb (2)
change(1-13)change(2-12)app/jobs/schedule_train_releases_job.rb (1)
perform(4-10)
app/components/release_list_component.rb (1)
app/models/train.rb (1)
next_run_at(219-252)
spec/models/train_spec.rb (3)
app/models/train.rb (1)
next_run_at(219-252)db/migrate/20230802131756_add_train_schedule.rb (2)
change(1-10)change(2-9)db/migrate/20230803115802_add_scheduled_releases_attempts_for_train.rb (2)
change(1-13)change(2-12)
app/models/train.rb (3)
app/components/scheduled_train_component.rb (1)
next_run_at(99-102)app/components/event_timeline_component.rb (1)
tz(73-73)app/models/build_artifact.rb (1)
app(60-62)
🪛 GitHub Actions: CI
db/migrate/20251118165553_change_kickoff_at_to_kickoff_time.rb
[error] 1-1: Pending migration detected. Run 'bin/rails db:migrate' to apply migrations.
🪛 GitHub Check: lint
spec/factories/trains.rb
[failure] 52-52:
Rails/DurationArithmetic: Do not add or subtract duration.
spec/models/train_spec.rb
[failure] 200-200:
Rails/DurationArithmetic: Do not add or subtract duration.
app/models/train.rb
[failure] 243-243:
Style/StringLiteralsInInterpolation: Prefer double-quoted strings inside interpolations.
🔇 Additional comments (7)
lib/tasks/anonymize.rake (1)
45-59: kickoff_time whitelist matches new schemaUpdating the trains whitelist to use
"kickoff_time"instead of"kickoff_at"is consistent with the migration and preserves the scheduling data shape in anonymized dumps; no functional issues here.app/components/release_list_component.rb (1)
145-155: Timezone-aware upcoming-release message looks correctUsing
train.next_run_at.in_time_zone(train.app.timezone).to_fs(:short)here aligns the UI with the new kickoff_time semantics and keeps the “Upcoming release” copy in the app’s local timezone. Given thetrain.automatic?guard and schedule validations, relying onnext_run_atbeing present in this branch is reasonable.db/data/20240305124235_connect_scheduled_releases_with_release.rb (1)
4-7: kickoff_time filter is consistent (though currently unreachable)Updating the automatic-train scope to
.where.not(kickoff_time: nil)matches the new schema. Given the earlyreturninup, this migration still does nothing at runtime, but the dormant logic is now consistent if it’s ever re-enabled.spec/components/scheduled_train_component_spec.rb (1)
44-44: Kickoff time test data wiring looks consistent with new time-of-day semanticsUsing
kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S")in these CalVer scenarios matches the newkickoff_time(HH:MM:SS) contract and keeps the relative schedule logic intact with the updatednext_run_atimplementation. No issues from a correctness standpoint.Also applies to: 69-69, 96-96, 121-121, 174-174, 199-199, 226-226, 251-251
app/controllers/trains_controller.rb (1)
95-113: Controller wiring forkickoff_timeand release schedule config looks correctAllowing
:kickoff_timein both create/update params and threading it throughrelease_schedule_config_params/release_schedule_configkeeps the controller aligned with the new time-of-day–based scheduling logic. Passing through the raw value here and letting the model/DB handle casting is appropriate.Also applies to: 158-172, 246-258
app/views/trains/_release_schedule_form.html.erb (1)
14-17: Form changes correctly reflectkickoff_timeand new Stimulus targetSwitching to
labeled_time_field :kickoff_timewith the"Kickoff time"label and updating the Stimulus target to"kickoffTime"are consistent with the new time-of-day scheduling model and the controller’s permitted params. This keeps the UI aligned with the backend behavior.spec/models/train_spec.rb (1)
100-107:next_run_atand activation specs accurately capture the new scheduling semanticsThe updated specs around
activate!and#next_run_atcover the key behaviors of the new time-based scheduling:
- First scheduled release aligns with
kickoff_timein the app’s timezone.- When no releases exist,
next_run_atis the next occurrence of kickoff_time (today or rolled forward by repeat_duration).- When releases exist,
next_run_atadvances from the lastscheduled_at, including when that last run is far in the past.The use of
in_time_zone(train.app.timezone).strftime("%H:%M:%S")for assertions is consistent with the Train implementation and ensures the tests are robust to timezone handling. No functional issues here.Also applies to: 111-172
| def next_run_at | ||
| return unless automatic? | ||
|
|
||
| base_time = last_run_at | ||
| now = Time.current | ||
| # Get the app's timezone | ||
| tz = app.timezone | ||
|
|
||
| return base_time if now < base_time | ||
| # If we have previous scheduled releases, calculate next occurrence from last scheduled time | ||
| if scheduled_releases.any? | ||
| base_time = scheduled_releases.last.scheduled_at | ||
| next_time = base_time + repeat_duration | ||
|
|
||
| time_difference = now - base_time | ||
| elapsed_durations = (time_difference / repeat_duration.to_i).ceil | ||
| base_time + (repeat_duration.to_i * elapsed_durations) | ||
| # Keep adding repeat_duration until we get a future time | ||
| while next_time <= Time.current | ||
| next_time += repeat_duration | ||
| end | ||
|
|
||
| return next_time.utc | ||
| end | ||
|
|
||
| # First time: combine kickoff_time with today's date in app timezone | ||
| now_in_tz = Time.current.in_time_zone(tz) | ||
|
|
||
| # Parse kickoff_time as a time today in the app's timezone | ||
| # kickoff_time is stored as HH:MM:SS | ||
| kickoff_today = tz.parse("#{now_in_tz.to_date} #{kickoff_time.strftime('%H:%M:%S')}") | ||
|
|
||
| # If kickoff_today already passed, start from the next occurrence | ||
| next_time = kickoff_today | ||
| while next_time <= now_in_tz | ||
| next_time += repeat_duration | ||
| end | ||
|
|
||
| next_time.utc | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure recurring runs stay at the same local time across DST + address lint hint
The new next_run_at correctly uses app.timezone for the first occurrence, but when there are existing scheduled_releases it does:
base_time = scheduled_releases.last.scheduled_at
next_time = base_time + repeat_duration
while next_time <= Time.current
next_time += repeat_duration
endBecause scheduled_at is stored in UTC, subsequent runs are computed purely in UTC. On DST transitions this will shift the wall-clock time (e.g., daily runs may move by ±1 hour), which conflicts with the stated goal of “same local time regardless of DST”.
You can keep all recurrence math in the app’s timezone (for both first and subsequent runs) and only convert to UTC at the end. This also lets you reuse the same now_in_tz and fixes the string-literal-in-interpolation lint by switching to double quotes:
def next_run_at
return unless automatic?
- # Get the app's timezone
- tz = app.timezone
-
- # If we have previous scheduled releases, calculate next occurrence from last scheduled time
- if scheduled_releases.any?
- base_time = scheduled_releases.last.scheduled_at
- next_time = base_time + repeat_duration
-
- # Keep adding repeat_duration until we get a future time
- while next_time <= Time.current
- next_time += repeat_duration
- end
-
- return next_time.utc
- end
-
- # First time: combine kickoff_time with today's date in app timezone
- now_in_tz = Time.current.in_time_zone(tz)
-
- # Parse kickoff_time as a time today in the app's timezone
- # kickoff_time is stored as HH:MM:SS
- kickoff_today = tz.parse("#{now_in_tz.to_date} #{kickoff_time.strftime('%H:%M:%S')}")
-
- # If kickoff_today already passed, start from the next occurrence
- next_time = kickoff_today
- while next_time <= now_in_tz
- next_time += repeat_duration
- end
-
- next_time.utc
+ tz = app.timezone
+ now_in_tz = Time.current.in_time_zone(tz)
+
+ if scheduled_releases.any?
+ # Work in app timezone so recurring runs stay at the same local time
+ base_time_in_tz = scheduled_releases.last.scheduled_at.in_time_zone(tz)
+ next_time_in_tz = base_time_in_tz + repeat_duration
+
+ while next_time_in_tz <= now_in_tz
+ next_time_in_tz += repeat_duration
+ end
+
+ return next_time_in_tz.utc
+ end
+
+ # First time: combine kickoff_time with today's date in app timezone
+ kickoff_today = tz.parse("#{now_in_tz.to_date} #{kickoff_time.strftime("%H:%M:%S")}")
+
+ next_time_in_tz = kickoff_today
+ while next_time_in_tz <= now_in_tz
+ next_time_in_tz += repeat_duration
+ end
+
+ next_time_in_tz.utc
endThis keeps all scheduled runs anchored to the same local wall-clock time in each app’s timezone and also satisfies the StringLiteralsInInterpolation lint.
I'll verify the review comment by examining the actual code and understanding the timezone/DST concerns.
🏁 Script executed:
#!/bin/bash
# Examine the current next_run_at implementation
cat app/models/train.rb | sed -n '219,252p'Length of output: 1030
🏁 Script executed:
#!/bin/bash
# Check the full context around the method and related timezone logic
cat app/models/train.rb | sed -n '200,260p'Length of output: 1655
🏁 Script executed:
#!/bin/bash
# Find related timezone and scheduling code
rg -n "scheduled_releases|kickoff_time|repeat_duration" app/models/train.rb -B2 -A2Length of output: 4059
🏁 Script executed:
#!/bin/bash
# Check if there are tests for next_run_at behavior
fd "train" app/models --type f | xargs rg -l "next_run_at" 2>/dev/null | head -5Length of output: 19
🏁 Script executed:
#!/bin/bash
# Find test files for train model
fd ".*train.*test\|.*train.*spec" --type f | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the Migration or schema to confirm scheduled_at storage
rg -n "scheduled_at" --type ruby app/models app/migrations -B2 -A2 | head -20Length of output: 1078
🏁 Script executed:
#!/bin/bash
# Verify the Ruby lint rule about string literals in interpolation
rg -n "StringLiteralsInInterpolation" . --type ruby 2>/dev/null | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check repeat_duration data type and how it's used
rg -n "repeat_duration" app/models/train.rb -B3 -A3Length of output: 2907
🏁 Script executed:
#!/bin/bash
# Check Time.current vs now_in_tz usage for consistency
rg -n "Time\.current|now_in_tz" app/models/train.rb -B1 -A1Length of output: 544
🏁 Script executed:
#!/bin/bash
# Verify automatic? method definition and its dependencies
rg -n "def automatic\?|kickoff_time\.|repeat_duration\." app/models/train.rb -B1 -A2 | head -30Length of output: 1127
🏁 Script executed:
#!/bin/bash
# Check if there's a timezone attribute on the app model
rg -n "def timezone|timezone" app/models/app.rb -B1 -A2 | head -20Length of output: 240
🏁 Script executed:
#!/bin/bash
# Look for DST-related tests or comments in the codebase
rg -i "dst|daylight.*saving" app/models app/spec -C3 2>/dev/nullLength of output: 5199
🏁 Script executed:
#!/bin/bash
# Examine the scheduled_releases association and scheduled_at attribute
rg -n "has_many.*scheduled_releases|class ScheduledRelease" app/models/train.rb app/models/scheduled_release.rb -B2 -A5Length of output: 1083
🏁 Script executed:
#!/bin/bash
# Check the ScheduledRelease model structure
test -f app/models/scheduled_release.rb && cat app/models/scheduled_release.rb | head -50Length of output: 1358
🏁 Script executed:
#!/bin/bash
# Verify if scheduled_at is a datetime stored in UTC
rg -n "scheduled_at" app/models/scheduled_release.rb -B2 -A2Length of output: 997
🏁 Script executed:
#!/bin/bash
# Look for any existing timezone conversion in the code
rg -n "in_time_zone|\.utc|\.local" app/models/train.rb -B1 -A1 | head -40Length of output: 222
🏁 Script executed:
#!/bin/bash
# Check for Rails version and available timezone utilities
test -f Gemfile && grep -i "rails\|activesupport" Gemfile | head -5Length of output: 144
🏁 Script executed:
#!/bin/bash
# Verify lint configuration
test -f .rubocop.yml && cat .rubocop.yml | grep -A10 "StringLiteralsInInterpolation" 2>/dev/null || echo "No .rubocop.yml or rule not found"Length of output: 33
🏁 Script executed:
#!/bin/bash
# Check the actual diff to understand what changed
git diff HEAD^ app/models/train.rb | head -100Length of output: 4062
🏁 Script executed:
#!/bin/bash
# Review the validation and callback for kickoff_time changes
rg -n "valid_schedule|schedule_release\!" app/models/train.rb -B3 -A5Length of output: 130
🏁 Script executed:
#!/bin/bash
# Check if runnable? method uses next_run_at
rg -n "def runnable\?" app/models/train.rb -B2 -A10Length of output: 312
🏁 Script executed:
#!/bin/bash
# Look for any existing DST handling or timezone-aware scheduling
rg -n "DST|next_occurrence|scheduled.*time" app/models/ -i --type ruby | head -20Length of output: 1550
🏁 Script executed:
#!/bin/bash
# Verify the exact timezone attribute structure
rg -n "timezone" app/models/ -C2 --type ruby | grep -E "(attr|column|def)" | head -15Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the parsing logic to ensure tz.parse is valid
rg -n "tz\.parse|ActiveSupport::TimeZone" app/models/train.rb -B1 -A1Length of output: 143
🏁 Script executed:
#!/bin/bash
# Look at the proposed change more carefully - verify in_time_zone chaining
ast-grep --pattern 'in_time_zone($_)'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Double-check if Time values stored in DB are UTC by default in Rails
rg -n "create_table.*scheduled_release\|t\.datetime" db/migrate -B5 -A10 | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify repeat_duration is stored as a duration/interval
rg -n "repeat_duration" app/models/train.rb | head -5Length of output: 356
🏁 Script executed:
#!/bin/bash
# Check how repeat_duration is used in addition operations
rg -n "repeat_duration" app/models/train.rb -A5 -B1 | grep -E "(\+|-|\*|repeat)" | head -10Length of output: 680
🏁 Script executed:
#!/bin/bash
# Check if there are any existing issues or TODOs related to timezone handling
rg -n "TODO|FIXME|BUG|XXX" app/models/train.rb | grep -i "time\|zone\|dst"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify the exact line numbers and check if code matches the review
sed -n '219,252p' app/models/train.rb | wc -lLength of output: 2
🏁 Script executed:
#!/bin/bash
# Get more context - check the entire next_run_at method
ast-grep --pattern 'def next_run_at { $$$ }'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Last validation - verify strftime usage with quotes
grep -n "strftime.*%" app/models/train.rb | head -5Length of output: 93
🏁 Script executed:
#!/bin/bash
# Check if the current branch/PR actually contains the method in question
git log --oneline -1 && git diff --name-only HEAD~1 HEAD 2>/dev/null | grep train.rbLength of output: 97
🏁 Script executed:
#!/bin/bash
# See if the original comment hints at what was before this change
git show HEAD:app/models/train.rb 2>/dev/null | sed -n '219,252p' || echo "Cannot access previous version"Length of output: 1030
🏁 Script executed:
#!/bin/bash
# Inspect the full train.rb file structure
wc -l app/models/train.rbLength of output: 23
🏁 Script executed:
#!/bin/bash
# Ensure file exists
test -f app/models/train.rb && echo "File exists" || echo "File does not exist"Length of output: 11
🏁 Script executed:
#!/bin/bash
# List the app/models directory
ls -la app/models/ | head -20Length of output: 1287
🏁 Script executed:
#!/bin/bash
# Check if this is the right repo structure
pwd && ls -la | head -20Length of output: 1222
🏁 Script executed:
#!/bin/bash
# Verify git status
git status --short 2>/dev/null | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get repo info
git remote -v 2>/dev/null || echo "No git remote"Length of output: 126
🏁 Script executed:
#!/bin/bash
# Check branch info
git branch -v 2>/dev/null || echo "Not a git repo or no branches"Length of output: 172
🏁 Script executed:
#!/bin/bash
# Try to see recent changes
git log --oneline -5 2>/dev/null || echo "Cannot access git log"Length of output: 334
🏁 Script executed:
#!/bin/bash
# List all Ruby model files
find app/models -name "*.rb" -type f 2>/dev/nullLength of output: 2898
🏁 Script executed:
#!/bin/bash
# Final check: examine the actual code structure
cat app/models/train.rb 2>/dev/null | head -50 || echo "Cannot read file"Length of output: 3442
🏁 Script executed:
#!/bin/bash
# Alternative: use find to locate the file
find . -name "train.rb" -path "*/models/*" 2>/dev/nullLength of output: 21
🏁 Script executed:
#!/bin/bash
# Check current directory structure
ls -laLength of output: 3149
🏁 Script executed:
#!/bin/bash
# Check if we have access to app directory
test -d app && ls -la app/ || echo "No app directory"Length of output: 930
🏁 Script executed:
#!/bin/bash
# Final sanity check
pwd && find . -maxdepth 3 -type d -name models 2>/dev/nullLength of output: 45
🏁 Script executed:
#!/bin/bash
# List everything we can find
ls -R . | head -100 || echo "Cannot list"Length of output: 1101
🏁 Script executed:
#!/bin/bash
# Check environment
env | grep -i ruby || echo "Check failed"Length of output: 12
🏁 Script executed:
#!/bin/bash
# Simple directory listing
ls 2>/dev/null || pwdLength of output: 281
Recurring runs will drift from kickoff time across DST transitions; consolidate timezone handling and fix lint issue
The new next_run_at correctly uses app.timezone for the first occurrence, but when scheduled_releases exist, it performs all recurrence math in UTC:
base_time = scheduled_releases.last.scheduled_at # UTC datetime
next_time = base_time + repeat_duration # UTC math
while next_time <= Time.current # UTC comparisonBecause scheduled_at is stored as UTC, subsequent runs bypass timezone logic. On DST transitions this shifts the wall-clock time (e.g., a daily 9 AM run may become 10 AM after spring forward), contradicting the stated goal of "same local time regardless of DST".
Convert the scheduled_releases branch to use app timezone for all recurrence math and only convert to UTC at the end:
def next_run_at
return unless automatic?
- # Get the app's timezone
- tz = app.timezone
-
- # If we have previous scheduled releases, calculate next occurrence from last scheduled time
- if scheduled_releases.any?
- base_time = scheduled_releases.last.scheduled_at
- next_time = base_time + repeat_duration
-
- # Keep adding repeat_duration until we get a future time
- while next_time <= Time.current
- next_time += repeat_duration
- end
-
- return next_time.utc
- end
-
- # First time: combine kickoff_time with today's date in app timezone
- now_in_tz = Time.current.in_time_zone(tz)
-
- # Parse kickoff_time as a time today in the app's timezone
- # kickoff_time is stored as HH:MM:SS
- kickoff_today = tz.parse("#{now_in_tz.to_date} #{kickoff_time.strftime('%H:%M:%S')}")
-
- # If kickoff_today already passed, start from the next occurrence
- next_time = kickoff_today
- while next_time <= now_in_tz
- next_time += repeat_duration
- end
-
- next_time.utc
+ tz = app.timezone
+ now_in_tz = Time.current.in_time_zone(tz)
+
+ if scheduled_releases.any?
+ # Work in app timezone so recurring runs stay at the same local time
+ base_time_in_tz = scheduled_releases.last.scheduled_at.in_time_zone(tz)
+ next_time_in_tz = base_time_in_tz + repeat_duration
+
+ while next_time_in_tz <= now_in_tz
+ next_time_in_tz += repeat_duration
+ end
+
+ return next_time_in_tz.utc
+ end
+
+ # First time: combine kickoff_time with today's date in app timezone
+ kickoff_today = tz.parse("#{now_in_tz.to_date} #{kickoff_time.strftime("%H:%M:%S")}")
+
+ next_time_in_tz = kickoff_today
+ while next_time_in_tz <= now_in_tz
+ next_time_in_tz += repeat_duration
+ end
+
+ next_time_in_tz.utc
endThis keeps all scheduled runs anchored to the same local wall-clock time in the app's timezone and also addresses the StringLiteralsInInterpolation lint hint by using double quotes.
🧰 Tools
🪛 GitHub Check: lint
[failure] 243-243:
Style/StringLiteralsInInterpolation: Prefer double-quoted strings inside interpolations.
🤖 Prompt for AI Agents
In app/models/train.rb around lines 219-252, the scheduled_releases branch does
recurrence math in UTC which causes DST drift and the interpolation uses
single-quoted string literals; convert the last scheduled_at into the app
timezone and do all recurrence additions and comparisons in that Time.zone (e.g.
base_time_in_tz = scheduled_releases.last.scheduled_at.in_time_zone(tz)), loop
adding repeat_duration to that time until it's after now_in_tz (use now_in_tz =
Time.current.in_time_zone(tz)), then convert the resulting TimeWithZone to UTC
for return; also change any string literal in interpolation (e.g. the
kickoff_time.strftime format) to use double quotes to satisfy
StringLiteralsInInterpolation.
| reversible do |dir| | ||
| dir.up do | ||
| Train.reset_column_information | ||
| Train.find_each do |train| | ||
| next if train.kickoff_at.blank? | ||
|
|
||
| # Convert kickoff_at (UTC) to app's timezone and extract time component | ||
| time_in_app_tz = train.kickoff_at.in_time_zone(train.app.timezone) | ||
| train.update_column(:kickoff_time, time_in_app_tz.strftime("%H:%M:%S")) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
Down-migration parsing bug for kickoff_time + minor migration cleanup
Two points here:
-
Down path: malformed parse string (bug)
In the down direction you build the datetime with:kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time}")
For a
timecolumn,train.kickoff_timewill typically stringify with its own date and zone, so the resulting string is ambiguous (e.g.,"2025-11-19 2000-01-01 11:00:00 UTC"), andtz.parsemay misinterpret or fail. You already do the right thing inTrain#next_run_atby formatting only the time portion.I’d strongly recommend mirroring that here:
-
kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time}")
-
kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time.strftime("%H:%M:%S")}")This makes the down migration deterministic and matches the semantics used in the model.
-
Optional: avoid N+1 on app lookups
Both up and down useTrain.find_eachand then hittrain.app.timezone, which will issue an extra query per train. If you expect a large trains table, you can cheaply remove this N+1:
-
Train.reset_column_information -
Train.find_each do |train|
-
Train.reset_column_information -
Train.includes(:app).find_each do |train|(Same pattern in the down block.) This keeps behavior identical but scales better on larger datasets.
Also applies to: 34-52
🏁 Script executed:
#!/bin/bash
# Retrieve the full migration file to inspect down block
find . -name "20251118165553_change_kickoff_at_to_kickoff_time.rb" -type f -exec cat {} \;Length of output: 1764
🏁 Script executed:
#!/bin/bash
# Find Train model to check next_run_at method
fd -e rb -path "*/models/*" -name "*train*" -type f | head -20Length of output: 168
🏁 Script executed:
#!/bin/bash
# Search for next_run_at method in Train model
rg -n "def next_run_at" --type=rb -A 5Length of output: 26
🏁 Script executed:
#!/bin/bash
# Check how time columns are typically used in migrations/models
rg -n "\.strftime\(" --type=rb -C 2 | grep -A 2 -B 2 "time"Length of output: 26
🏁 Script executed:
#!/bin/bash
# Find Train model files
find . -name "*train*" -type f | grep -E "models?/" | head -10Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for next_run_at method without type flag
rg -n "def next_run_at" -A 5Length of output: 649
🏁 Script executed:
#!/bin/bash
# Search for how time columns are used in codebase
rg -n "kickoff_time" -C 3Length of output: 20885
🏁 Script executed:
#!/bin/bash
# Look for strftime usage with time values to understand pattern
rg "strftime.*%H:%M:%S" -C 2Length of output: 8379
Down-migration parsing bug for kickoff_time + minor migration cleanup
Two points here:
-
Down path: malformed parse string (bug)
In the down direction you build the datetime with:kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time}")
For a
timecolumn,train.kickoff_timewhen interpolated lacks explicit formatting. The codebase consistently usesstrftime("%H:%M:%S")when parsing—as seen inTrain#next_run_at(line 243) and throughout specs. Update to match this pattern:
-
kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time}")
-
kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time.strftime("%H:%M:%S")}")This ensures the down migration is deterministic and consistent with the model logic.
-
Optional: avoid N+1 on app lookups
Both up and down useTrain.find_eachand then hittrain.app.timezone, which issues an extra query per train. Add eager loading:
-
Train.reset_column_information -
Train.find_each do |train|
-
Train.reset_column_information -
Train.includes(:app).find_each do |train|(Same pattern in the down block.)
🤖 Prompt for AI Agents
In db/migrate/20251118165553_change_kickoff_at_to_kickoff_time.rb around lines
10 to 21, the down migration builds a datetime using an unformatted kickoff_time
string which can parse inconsistently; change the interpolation to use the same
formatting pattern used elsewhere (strftime("%H:%M:%S")) when reconstructing the
time so parsing is deterministic, and to avoid N+1 queries preload apps for both
up and down (use a query that eager-loads the associated app before iterating,
e.g., include the app association) so train.app.timezone does not hit the DB per
record.
- Add kickoff_datetime helper method to centralize timezone conversion - Restore original mathematical approach instead of while loops - Calculate elapsed durations and jump ahead directly (more efficient) - Remove redundant timezone math scattered throughout the code
Changed kickoff_at (datetime) to kickoff_time (time) to make release scheduling based on time of day rather than specific datetime. The scheduled_at for ScheduledRelease is still stored in UTC but is now calculated by combining kickoff_time with the app's configured timezone.
This makes scheduling more intuitive - trains kick off at the same time of day in the app's timezone regardless of DST changes or other timezone complications.
Changes:
BREAKING CHANGE: Existing trains with kickoff_at will need to run the migration to convert to kickoff_time
Closes: https://github.com/tramlinehq/site/issues/
Why
Enter the reason/premise for raising this PR.
This addresses
Enter the details of what all this covers, include screenshots, demos, thought processes etc.
Scenarios tested
Summary by CodeRabbit
Bug Fixes
Chores