chore: release schedule should be timezone agnostic #888
Conversation
WalkthroughThis PR converts trains.kickoff_at from a time to a naive datetime in the DB via two migrations, disables ActiveRecord timezone conversion for kickoff_at, and adds normalization and accessor logic in Train: a kickoff_at= setter, kickoff_datetime (interprets stored naive datetime in app timezone), and updates last_run_at and schedule validation to use kickoff_datetime. The controller/helper now use kickoff_datetime for formatting/config, tests for DST-safe scheduling were added, and release_list rendering adjusted (no_releases? and button type change). Sequence Diagram(s)sequenceDiagram
participant User
participant Controller as TrainsController
participant Model as Train (kickoff_at= / kickoff_datetime)
participant DB as Database (trains.kickoff_at)
participant View as Helper/View
User->>Controller: submit create/update with kickoff_at (String/Time)
Controller->>Model: assign attrs (calls kickoff_at=)
Model->>Model: kickoff_at= normalizes → naive UTC timestamp
Model->>DB: persist kickoff_at (naive datetime)
Note right of DB: Migrations convert column types<br/>and transform stored values
DB-->>Model: return persisted kickoff_at
Model->>Model: kickoff_datetime interprets kickoff_at in app timezone
Model->>Controller: provide kickoff_datetime / validators / last_run_at
Controller->>View: render using Helper (release_schedule uses kickoff_datetime)
View->>User: display formatted local date/time
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/controllers/trains_controller.rb(1 hunks)app/helpers/application_helper.rb(1 hunks)app/models/train.rb(3 hunks)db/migrate/20251208093103_change_kickoff_at_to_time_in_trains.rb(1 hunks)db/migrate/20251208102246_change_kickoff_at_to_datetime_in_trains.rb(1 hunks)db/schema.rb(7 hunks)spec/models/train_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/helpers/application_helper.rb (1)
app/models/train.rb (1)
kickoff_datetime(269-288)
spec/models/train_spec.rb (1)
app/models/train.rb (2)
kickoff_datetime(269-288)last_run_at(249-251)
db/migrate/20251208102246_change_kickoff_at_to_datetime_in_trains.rb (1)
db/migrate/20251208093103_change_kickoff_at_to_time_in_trains.rb (3)
up(1-25)up(2-12)down(14-24)
db/migrate/20251208093103_change_kickoff_at_to_time_in_trains.rb (1)
db/migrate/20251208102246_change_kickoff_at_to_datetime_in_trains.rb (3)
up(1-41)up(2-20)down(22-40)
🪛 GitHub Actions: Lint
db/schema.rb
[warning] 1-1: Schema file changed in commit without an update in lib/tasks/anonymize.rake afterward.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (10)
app/controllers/trains_controller.rb (1)
259-259: LGTM! Storing kickoff_at directly aligns with the new timezone-aware model logic.The controller now delegates timezone handling to the
Trainmodel's customkickoff_at=setter, which normalizes the input into a naive UTC timestamp. This is then interpreted as local time viakickoff_datetimewhen needed.app/helpers/application_helper.rb (1)
156-167: LGTM! Helper correctly uses timezone-aware kickoff_datetime.The helper now uses
kickoff_datetimewhich interprets the naivekickoff_atvalue in the app's timezone, ensuring consistent local time display regardless of DST changes. The formatting maintains the existing output structure.spec/models/train_spec.rb (1)
695-787: LGTM! Comprehensive DST-safe scheduling tests.The test suite thoroughly covers:
kickoff_datetimeinterpretation in app timezone- Nil handling
- Multiple timezone support (EST/EDT vs PDT)
- DST transitions in both directions (spring and fall)
last_run_atrelying onkickoff_datetimeThese tests provide strong confidence that the timezone-aware scheduling works correctly across DST boundaries.
app/models/train.rb (5)
55-55: LGTM! Disabling automatic timezone conversion is necessary.Setting
skip_time_zone_conversion_for_attributes = [:kickoff_at]prevents Rails from automatically converting thekickoff_atvalue, allowing the custom setter andkickoff_datetimemethod to handle timezone interpretation explicitly.
253-267: LGTM! Custom setter correctly normalizes kickoff_at to naive UTC.The setter handles both String and Time inputs, extracting the date/time components and storing them as a naive UTC timestamp. This preserves the "clock time" (e.g., "2 PM") without timezone information, allowing
kickoff_datetimeto later interpret it in the app's timezone.Edge case handling:
- Nil values are handled correctly (line 254)
- String parsing uses
Time.zone.parsewhich respects the current zone- Time objects extract components directly
269-288: LGTM! kickoff_datetime correctly interprets naive time in app timezone.The implementation interprets the stored naive
kickoff_atas a local time in the app's timezone, which ensures consistent scheduling across DST transitions. The approach of extracting components and rebuilding in the timezone context (lines 279-287) is correct.Key insight from line 277: "timezone reflects current DST rules rather than stored date"
- This is intentional and correct for scheduling
- A 2 PM kickoff remains 2 PM local time regardless of DST
- Tests verify this behavior across DST boundaries
249-251: LGTM! last_run_at correctly uses kickoff_datetime.The method now returns the timezone-aware
kickoff_datetimeinstead of the naivekickoff_at, ensuring that scheduling calculations work correctly with proper timezone context.
634-641: LGTM! Validation correctly uses kickoff_datetime for future check.Line 638 now uses
kickoff_datetimeinstead ofkickoff_atfor the future time validation, ensuring the comparison is made in the app's timezone rather than UTC. This prevents validation errors due to timezone mismatches.db/schema.rb (2)
1068-1068: LGTM! Schema correctly reflects kickoff_at as datetime.The
kickoff_atcolumn is now properly defined asdatetime, which aligns with the PR objectives and the migration changes.
13-13: No action needed:kickoff_atis already whitelisted in anonymize.rake.The trains table in
lib/tasks/anonymize.rakealready includes"kickoff_at"in the whitelist (line with trains table definition), so the anonymization task correctly preserves this field during data copying. Sincekickoff_atis a timestamp and not PII, keeping it as-is is the appropriate handling.Likely an incorrect or invalid review comment.
| class ChangeKickoffAtToTimeInTrains < ActiveRecord::Migration[7.2] | ||
| def up | ||
| safety_assured do | ||
| # Convert existing UTC datetimes to naive local datetimes | ||
| # This extracts the local time component and stores it without timezone info | ||
| execute <<~SQL | ||
| UPDATE trains | ||
| SET kickoff_at = (kickoff_at AT TIME ZONE 'UTC')::timestamp | ||
| WHERE kickoff_at IS NOT NULL | ||
| SQL | ||
| end | ||
| end | ||
|
|
||
| def down | ||
| # This is lossy - we can't perfectly recreate the original UTC times | ||
| # But we can convert back to UTC assuming current timezone | ||
| safety_assured do | ||
| execute <<~SQL | ||
| UPDATE trains | ||
| SET kickoff_at = kickoff_at AT TIME ZONE 'UTC' | ||
| WHERE kickoff_at IS NOT NULL | ||
| SQL | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for any other migrations affecting kickoff_at
echo "=== All migrations mentioning kickoff_at ==="
rg -l "kickoff_at" db/migrate/
echo -e "\n=== Migration sequence ==="
ls -1 db/migrate/*kickoff* 2>/dev/null | sort
echo -e "\n=== Check for ALTER COLUMN statements ==="
rg "alter.*kickoff_at|change_column.*kickoff_at" db/migrate/ -iRepository: tramlinehq/tramline
Length of output: 599
🏁 Script executed:
cat db/migrate/20251208102246_change_kickoff_at_to_datetime_in_trains.rbRepository: tramlinehq/tramline
Length of output: 1130
🏁 Script executed:
cat -n db/migrate/20251208093103_change_kickoff_at_to_time_in_trains.rbRepository: tramlinehq/tramline
Length of output: 989
🏁 Script executed:
cat -n db/migrate/20251208102246_change_kickoff_at_to_datetime_in_trains.rbRepository: tramlinehq/tramline
Length of output: 1417
Migration 093103 must explicitly change the column type to time using change_column.
The first migration converts values using AT TIME ZONE 'UTC' cast but does not change the kickoff_at column type from timestamp to time. The subsequent migration 102246 immediately uses CURRENT_DATE + kickoff_at on line 10, which requires kickoff_at to be a time type. In PostgreSQL, date + timestamp is an invalid operation; only date + time is valid and produces a timestamp. Without an explicit type change in migration 093103, migration 102246 will fail at runtime.
Add this after the SQL update in migration 093103's up block:
change_column :trains, :kickoff_at, :timeAnd in the down block, change back:
change_column :trains, :kickoff_at, :datetime🤖 Prompt for AI Agents
In db/migrate/20251208093103_change_kickoff_at_to_time_in_trains.rb lines 1-25,
the migration only converts kickoff_at values but does not change the column
type to time, causing later migrations that expect a time to fail; after the
UPDATE in the up block add a change_column :trains, :kickoff_at, :time to alter
the column type, and in the down block after the UPDATE add change_column
:trains, :kickoff_at, :datetime to restore the original type (keep both inside
the existing safety_assured blocks).
| class ChangeKickoffAtToDatetimeInTrains < ActiveRecord::Migration[7.2] | ||
| def up | ||
| safety_assured do | ||
| # Add a temporary datetime column | ||
| add_column :trains, :kickoff_datetime, :datetime | ||
|
|
||
| # Convert time to datetime by combining with current date | ||
| execute <<~SQL | ||
| UPDATE trains | ||
| SET kickoff_datetime = CURRENT_DATE + kickoff_at | ||
| WHERE kickoff_at IS NOT NULL | ||
| SQL | ||
|
|
||
| # Remove the time column | ||
| remove_column :trains, :kickoff_at | ||
|
|
||
| # Rename the datetime column to kickoff_at | ||
| rename_column :trains, :kickoff_datetime, :kickoff_at | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all migration files related to kickoff_at
echo "=== Finding kickoff_at migrations ==="
fd ".*kickoff_at.*\.rb" db/migrate/ || echo "No files found by fd, trying ls..."
ls -la db/migrate/*kickoff_at* 2>/dev/null || echo "No matching files in db/migrate/"
# List all migrations to see the order
echo -e "\n=== All migrations in order ==="
ls -1 db/migrate/*.rb | sort | tail -20
# Check if the first migration exists
echo -e "\n=== Checking first migration (20251208093103) ==="
if [ -f "db/migrate/20251208093103_change_kickoff_at_to_time_in_trains.rb" ]; then
cat db/migrate/20251208093103_change_kickoff_at_to_time_in_trains.rb
else
echo "First migration not found"
fi
# Check schema.rb for kickoff_at column
echo -e "\n=== Checking schema.rb for kickoff_at column ==="
if [ -f "db/schema.rb" ]; then
grep -A2 -B2 "kickoff_at" db/schema.rb || echo "kickoff_at not found in schema.rb"
else
echo "schema.rb not found"
fiRepository: tramlinehq/tramline
Length of output: 2862
🏁 Script executed:
#!/bin/bash
# Find when kickoff_at was first added
echo "=== Searching for kickoff_at column creation ==="
rg "kickoff_at" db/migrate/*.rb --type rb -B2 -A2 | head -100
# Check git history for trains table schema
echo -e "\n=== Checking migrations that touch trains table ==="
rg "create_table :trains|add_column.*trains|change_column.*trains" db/migrate/*.rb --type rb -l
# Look for the initial creation of trains table
echo -e "\n=== Looking for trains table creation ==="
rg "create_table :trains" db/migrate/*.rb --type rb -B2 -A20Repository: tramlinehq/tramline
Length of output: 305
🏁 Script executed:
#!/bin/bash
# Search for kickoff_at without type flag
echo "=== Searching for kickoff_at in migrations ==="
rg "kickoff_at" db/migrate/ -B2 -A2 | head -150
# Also search for trains table creation and modifications
echo -e "\n=== Searching for trains table structure changes ==="
rg "create_table.*trains|add_column.*trains.*kickoff" db/migrate/ -B1 -A10Repository: tramlinehq/tramline
Length of output: 8247
🏁 Script executed:
#!/bin/bash
echo "=== Complete second migration file ==="
cat db/migrate/20251208102246_change_kickoff_at_to_datetime_in_trains.rb
echo -e "\n=== Complete first migration file ==="
cat db/migrate/20251208093103_change_kickoff_at_to_time_in_trains.rbRepository: tramlinehq/tramline
Length of output: 1960
Critical: Incorrect migration sequence with incompatible type assumptions.
The migration sequence has a fundamental flaw:
- First migration converts UTC datetime to local datetime but does not change the column type (remains
datetime) - Second migration assumes
kickoff_atis now atimetype (comment: "Convert time to datetime"), but the column is stilldatetime - The SQL
CURRENT_DATE + kickoff_atattempts to add a date to a datetime column, which is not a standard PostgreSQL operation and will likely fail or produce undefined behavior
The logic appears inverted: the first migration should convert datetime → time, and the second should convert time → datetime. As written, both migrations assume the column type has changed when it hasn't.
Recommendation: Reconsider the migration sequence:
- Clarify whether the final goal is
timeordatetimetype - Ensure the first migration explicitly changes the column type to
timeusingchange_column - Adjust the second migration's SQL accordingly, or reconsider if both migrations are necessary
- Add explicit type-change steps rather than relying on data-only conversions
🤖 Prompt for AI Agents
In db/migrate/20251208102246_change_kickoff_at_to_datetime_in_trains.rb lines
1-20, the migration assumes kickoff_at is a time when it is still a datetime and
uses invalid SQL (CURRENT_DATE + kickoff_at). Decide the intended final type
(datetime or time) and implement an explicit, type-safe conversion sequence: 1)
if final type is time, add a temp_time column, populate it by extracting the
time portion from the existing datetime (use proper SQL cast/extract),
swap/remove/rename columns and use change_column to set the final type; 2) if
final type is datetime, add a temp_datetime column, populate it by combining
CURRENT_DATE with the existing time using proper PostgreSQL functions (make sure
kickoff_at is time before combining), swap/remove/rename and use change_column
as needed; always use explicit change_column or temporary typed columns and safe
SQL casts to avoid relying on implicit incompatible operations.
| # It's strongly recommended that you check this file into your version control system. | ||
|
|
||
| ActiveRecord::Schema[7.2].define(version: 2025_12_01_162057) do | ||
| ActiveRecord::Schema[7.2].define(version: 2025_12_08_102246) do |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if these schema changes came from migrations in this PR
echo "=== Migrations in this PR ==="
git diff main --name-only -- db/migrate/ 2>/dev/null || echo "Note: Checking against origin/main"
echo -e "\n=== Schema changes summary ==="
git diff main -- db/schema.rb 2>/dev/null | grep -E "^\+\+\+|^---|^@@" | head -30
echo -e "\n=== Check migration files for kickoff_at ==="
find db/migrate -name "*.rb" -type f -exec grep -l "kickoff_at" {} \; 2>/dev/null || echo "No kickoff_at migrations found"
echo -e "\n=== Check migration files for flagged changes ==="
find db/migrate -name "*.rb" -type f -exec grep -l "commits\|notification_settings\|onboarding_states\|auto_rollout_enabled\|store_rollouts" {} \; 2>/dev/null || echo "No matching migrations found"
echo -e "\n=== List recent migrations ==="
ls -1 db/migrate/*.rb 2>/dev/null | tail -15Repository: tramlinehq/tramline
Length of output: 3680
🏁 Script executed:
#!/bin/bash
# Get the actual schema.rb diff to see the specific changes
echo "=== Schema diff around commits table (line 243) ==="
git diff main -- db/schema.rb 2>/dev/null | grep -A 5 -B 5 "commits.*index" | head -20
echo -e "\n=== Schema diff for notification_settings ==="
git diff main -- db/schema.rb 2>/dev/null | grep -A 3 -B 3 "notification_settings" | head -30
echo -e "\n=== Schema diff for onboarding_states ==="
git diff main -- db/schema.rb 2>/dev/null | grep -A 5 -B 5 "onboarding_states" | head -30
echo -e "\n=== Schema diff for auto_rollout_enabled ==="
git diff main -- db/schema.rb 2>/dev/null | grep -A 3 -B 3 "auto_rollout_enabled" | head -20
echo -e "\n=== Check if these tables/columns exist in main ==="
git show main:db/schema.rb 2>/dev/null | grep -c "onboarding_states" || echo "onboarding_states not in main"
git show main:db/schema.rb 2>/dev/null | grep -c "auto_rollout_enabled" || echo "auto_rollout_enabled not in main"Repository: tramlinehq/tramline
Length of output: 3443
🏁 Script executed:
#!/bin/bash
# Check for any uncommitted migration files
echo "=== Uncommitted migrations ==="
git status -- db/migrate/*.rb 2>/dev/null | head -20
echo -e "\n=== Check if main has the onboarding_states migration ==="
git log --oneline main -- db/migrate/ 2>/dev/null | grep -i "onboarding" | head -5
echo -e "\n=== Check if main has the auto_rollout migration ==="
git log --oneline main -- db/migrate/ 2>/dev/null | grep -i "auto_rollout\|store_rollout" | head -5
echo -e "\n=== Compare schema versions ==="
echo "Schema version in main:"
git show main:db/schema.rb 2>/dev/null | grep "Schema\[" | head -1
echo "Schema version in current branch:"
head -1 db/schema.rb | grep "Schema\["Repository: tramlinehq/tramline
Length of output: 427
Schema.rb contains unrelated changes without corresponding migrations in this PR or main branch.
The schema file includes several changes that are not generated by the two kickoff_at migrations in this PR:
notification_settingsconstraint: addedvalidate: falseparameteronboarding_statestable: new table withapp_id,data, timestamps, and indexstore_rollouts.auto_rollout_enabled: new boolean column- Foreign key
onboarding_states→apps
These changes are not present in main's schema or migrations. Before merging, investigate:
- Whether these schema changes were generated from migrations that should be in this PR but are missing
- If schema.rb was regenerated manually or via
rake db:schema:dumpafter database changes made outside of migrations - Ensure migrations and schema.rb are synchronized; if these changes are intentional, corresponding migration files must exist in the codebase
| it "correctly rejects past times" do | ||
| travel_to Time.zone.parse("2024-01-15 16:00:00") do # Current time: 4 PM | ||
| train.assign_attributes( | ||
| kickoff_at: "2024-01-15 15:00:00", # Scheduled time: 3 PM (1 hour in past) | ||
| repeat_duration: 1.day | ||
| ) | ||
|
|
||
| # Debug: check what kickoff_datetime returns | ||
| puts "Current time: #{Time.current}" | ||
| puts "kickoff_datetime: #{train.kickoff_datetime}" | ||
| puts "Is past?: #{train.kickoff_datetime <= Time.current}" | ||
|
|
||
| expect(train).not_to be_valid | ||
| expect(train.errors[:kickoff_at]).to include("the schedule kickoff should be in the future") | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Remove debug statements before merging.
Lines 808-810 contain debug puts statements that should be removed:
puts "Current time: #{Time.current}"
puts "kickoff_datetime: #{train.kickoff_datetime}"
puts "Is past?: #{train.kickoff_datetime <= Time.current}"These are helpful for debugging but should not be committed to the test suite.
Apply this diff to remove the debug statements:
travel_to Time.zone.parse("2024-01-15 16:00:00") do # Current time: 4 PM
train.assign_attributes(
kickoff_at: "2024-01-15 15:00:00", # Scheduled time: 3 PM (1 hour in past)
repeat_duration: 1.day
)
- # Debug: check what kickoff_datetime returns
- puts "Current time: #{Time.current}"
- puts "kickoff_datetime: #{train.kickoff_datetime}"
- puts "Is past?: #{train.kickoff_datetime <= Time.current}"
-
expect(train).not_to be_valid
expect(train.errors[:kickoff_at]).to include("the schedule kickoff should be in the future")
end🤖 Prompt for AI Agents
In spec/models/train_spec.rb around lines 800-816 (specifically remove the debug
prints at lines ~808-810), delete the three debug `puts` statements that output
Current time, kickoff_datetime and Is past? so the test contains only the setup,
the expectations, and no debugging output; ensure the surrounding
indentation/line breaks remain correct and run the test suite to confirm no
regressions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/components/release_list_component.html.erb (1)
91-103: Aligning delete visibility withno_releases?and button semanticsSwitching to
no_releases?for both the Remove button and the empty state keeps the view logic consistent and leaves room for richer “no releases” semantics (e.g., ignoring hotfixes or discarded releases). The change totype: :buttonalso matches the Activate/Deactivate buttons and is safer formethod: :delete+data: { turbo_method: :delete }, assumingButtonComponentmaps:buttonto a form/button submission variant.I’d just double‑check that:
no_releases?returnstrueexactly in the cases where the train is allowed to be deleted, and- it doesn’t trigger extra queries per train row (e.g., N+1 on
.releases).If both hold, this change looks solid.
app/models/train.rb (2)
55-55: Time zone conversion opt‑out forkickoff_atSetting
self.skip_time_zone_conversion_for_attributes = [:kickoff_at]matches the custom setter’s intent to treatkickoff_atas a naive timestamp and handle zone interpretation viakickoff_datetime.One minor consideration: this assignment replaces any existing
skip_time_zone_conversion_for_attributes. If you ever need to skip additional attributes in this model (or already do via inheritance), you may prefer:self.skip_time_zone_conversion_for_attributes += [:kickoff_at]to avoid accidentally discarding prior configuration.
233-243: DST handling is improved for kickoff, but repeat scheduling may still driftThe combination of:
kickoff_at=normalizing to a naive timestamp viaTime.utc(...), andkickoff_datetimereconstructing aTime.zone.localinapp.timezonenicely pins the initial kickoff to the intended local wall‑clock time regardless of DB/AR timezone behavior. Using
kickoff_datetimeinlast_run_atis consistent with that.There are a few edge cases worth double‑checking:
Parsing strings in the correct timezone
kickoff_at=parses strings withTime.zone.parse(time), butkickoff_datetimeexplicitly usesapp.timezone || Time.zone. IfTime.zoneis not already set toapp.timezonefor the controller/request, you could get a mismatch between how you parse and how you later interpret the same wall‑clock time.Consider changing the string branch to:
app_tz = app.timezone || Time.zone parsed = Time.use_zone(app_tz) { Time.zone.parse(time) }so both directions use the same timezone.
Handling malformed time strings
If
Time.zone.parse(time)returnsnil,parsed.yearwill raise. If that’s possible from user input, a defensive check with a validation error (rather than an exception) would be safer.Repeat schedule vs. DST
next_run_atstill advances usingrepeat_duration.to_i(seconds). Across DST boundaries, this will cause futurescheduled_atvalues to drift by one hour relative to the intended local kickoff time, even though the basekickoff_datetimeis now DST‑stable.If the product goal is “always at the same local time of day in the app’s timezone,” consider basing the recurrence on
kickoff_datetimewith calendar‑aware durations instead of raw seconds, e.g.:base_time = last_run_at elapsed_durations = (time_difference / repeat_duration.to_i).ceil next_time = elapsed_durations.times.reduce(base_time) { |t, _| t + repeat_duration }or similar logic that keeps using
ActiveSupport::Durationrather thanto_i.If your new specs already assert that weekly/daily schedules stay at the same clock time across DST changes, and they’re passing, it’s worth confirming that the test cases exercise this
next_run_atpath explicitly.Also applies to: 250-291
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/release_list_component.html.erb(1 hunks)app/models/train.rb(3 hunks)llm_rules.md(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/models/train.rb (1)
app/components/live_release/soak_component.rb (1)
app_timezone(41-43)
🪛 markdownlint-cli2 (0.18.1)
llm_rules.md
188-188: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
189-189: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
190-190: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
191-191: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
192-192: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
app/models/train.rb (1)
641-647: Schedule validation now uses app‑timezone interpretationSwitching the future‑time check to
kickoff_datetime <= Time.currentis consistent with the new semantics: users expect “is this in the future?” to be evaluated in the app’s local timezone, not raw stored UTC.Given:
kickoff_datetimeis derived inapp.timezone || Time.zone, andTime.currentis inTime.zone,Rails will compare them in UTC, so this should behave correctly as long as
Time.zoneis set appropriately per request (often to the app or organization timezone).You may want a spec that explicitly covers validation right around a DST boundary (e.g., just before/after the transition) to ensure no off‑by‑one‑hour surprises in edge cases.
| - Models (business logic, validations, state machines) | ||
| - Coordinators (orchestration) | ||
| - Jobs (async processing) | ||
| - Controllers (user input boundary) | ||
| - Views/Components (UI) |
There was a problem hiding this comment.
Fix unordered list indentation to satisfy markdownlint
The list under “Implement in layers” is indented by two spaces, which triggers MD007 (ul-indent) in markdownlint and may render inconsistently in some viewers.
You can fix this by unindenting the bullets to the left margin, e.g.:
- - Models (business logic, validations, state machines)
- - Coordinators (orchestration)
- - Jobs (async processing)
- - Controllers (user input boundary)
- - Views/Components (UI)
+- Models (business logic, validations, state machines)
+- Coordinators (orchestration)
+- Jobs (async processing)
+- Controllers (user input boundary)
+- Views/Components (UI)The rest of the new content (Justfile mention and Stimulus controller guidance) looks good and aligns with the described frontend stack.
📝 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.
| - Models (business logic, validations, state machines) | |
| - Coordinators (orchestration) | |
| - Jobs (async processing) | |
| - Controllers (user input boundary) | |
| - Views/Components (UI) | |
| - Models (business logic, validations, state machines) | |
| - Coordinators (orchestration) | |
| - Jobs (async processing) | |
| - Controllers (user input boundary) | |
| - Views/Components (UI) |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
188-188: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
189-189: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
190-190: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
191-191: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
192-192: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🤖 Prompt for AI Agents
In llm_rules.md around lines 188 to 192, the unordered list under "Implement in
layers" is indented by two spaces which triggers markdownlint MD007; remove the
two-space indentation so each bullet starts at the left margin (no leading
spaces) to satisfy markdownlint and ensure consistent rendering across viewers.
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.