Timezone-aware completion timestamps and location-based time #184

Merged

ldraney merged 6 commits from feature/timezone-aware-completion into main 2026-06-09 02:08:15 +00:00

Conversation 3 Commits 6 Files changed 16 +241 -13

ldraney commented 2026-06-09 01:36:51 +00:00

Owner

Copy link

Summary

• Work queue items now record completed_at timestamp and display "Finished at [time]" in the property's local timezone when checked off
• Properties store a time_zone (IANA string) set automatically during GPS detection, validated on write
• Browser timezone cookie drives Date.current so "Today" and all date logic respect the crew's actual location
• GPS detection JS now sends lat/lng + timezone to the server (previously captured but never persisted)
• rake dev now starts docker compose on port 7143 with automatic takeover -- stops whatever currently owns the port so the dev URL always points to the current checkout

Changes

• db/migrate/20260608100000_add_timezone_support.rb: adds completed_at (datetime) to work_queue_items, time_zone (string) to properties
• app/models/work_queue_item.rb: toggle_completion! sets/clears completed_at; finished_at_local converts to property timezone
• app/models/property.rb: local_time_zone helper, validates time_zone is recognized IANA string with 50-char length limit
• app/controllers/application_controller.rb: set_time_zone_from_browser before_action reads browser_tz cookie
• app/controllers/work_queue_items_controller.rb: uses toggle_completion! instead of manual boolean flip
• app/controllers/properties_controller.rb: permits :time_zone in strong params
• app/views/layouts/application.html.erb: inline script sets browser_tz cookie from Intl.DateTimeFormat
• app/views/work_queue_items/_queue_item.html.erb: shows "Finished at [time]" when completed, address when pending
• app/javascript/controllers/add_location_controller.js: sends latitude, longitude, and time_zone on GPS save
• Rakefile: dev task starts docker compose on :7143 with port takeover (stops existing container first)
• spec/models/work_queue_item_spec.rb: 5 new tests for toggle_completion! and finished_at_local
• spec/models/property_spec.rb: 3 new tests for timezone validation
• spec/requests/work_queue_items_spec.rb: updated toggle test + new uncomplete test

Test Plan

• 19 model specs pass (bundle exec rspec spec/models/work_queue_item_spec.rb spec/models/property_spec.rb)
• Check a property off on Today view -- shows "Finished at [time]" in local timezone
• Uncheck it -- reverts to showing the address
• Detect GPS on a property -- verify lat/lng and timezone are saved
• Navigate to Today from a non-Mountain timezone browser -- verify "Today" label is correct

Review Checklist

• Passed automated review-fix loop
• No secrets committed
• No unnecessary file changes
• Commit messages are descriptive
• Feature flag needed? No -- core data model improvement, not a toggleable workflow

Related Notes

• Closes #183
• landscaping-assistant -- project this affects

## Summary - Work queue items now record `completed_at` timestamp and display "Finished at [time]" in the property's local timezone when checked off - Properties store a `time_zone` (IANA string) set automatically during GPS detection, validated on write - Browser timezone cookie drives `Date.current` so "Today" and all date logic respect the crew's actual location - GPS detection JS now sends lat/lng + timezone to the server (previously captured but never persisted) - `rake dev` now starts docker compose on port 7143 with automatic takeover -- stops whatever currently owns the port so the dev URL always points to the current checkout ## Changes - `db/migrate/20260608100000_add_timezone_support.rb`: adds `completed_at` (datetime) to work_queue_items, `time_zone` (string) to properties - `app/models/work_queue_item.rb`: `toggle_completion!` sets/clears completed_at; `finished_at_local` converts to property timezone - `app/models/property.rb`: `local_time_zone` helper, validates time_zone is recognized IANA string with 50-char length limit - `app/controllers/application_controller.rb`: `set_time_zone_from_browser` before_action reads `browser_tz` cookie - `app/controllers/work_queue_items_controller.rb`: uses `toggle_completion!` instead of manual boolean flip - `app/controllers/properties_controller.rb`: permits `:time_zone` in strong params - `app/views/layouts/application.html.erb`: inline script sets `browser_tz` cookie from `Intl.DateTimeFormat` - `app/views/work_queue_items/_queue_item.html.erb`: shows "Finished at [time]" when completed, address when pending - `app/javascript/controllers/add_location_controller.js`: sends latitude, longitude, and time_zone on GPS save - `Rakefile`: `dev` task starts docker compose on :7143 with port takeover (stops existing container first) - `spec/models/work_queue_item_spec.rb`: 5 new tests for toggle_completion! and finished_at_local - `spec/models/property_spec.rb`: 3 new tests for timezone validation - `spec/requests/work_queue_items_spec.rb`: updated toggle test + new uncomplete test ## Test Plan - [x] 19 model specs pass (`bundle exec rspec spec/models/work_queue_item_spec.rb spec/models/property_spec.rb`) - [ ] Check a property off on Today view -- shows "Finished at [time]" in local timezone - [ ] Uncheck it -- reverts to showing the address - [ ] Detect GPS on a property -- verify lat/lng and timezone are saved - [ ] Navigate to Today from a non-Mountain timezone browser -- verify "Today" label is correct ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive - [x] Feature flag needed? No -- core data model improvement, not a toggleable workflow ## Related Notes - Closes #183 - `landscaping-assistant` -- project this affects

ldraney added 1 commit 2026-06-09 01:36:51 +00:00

Timezone-aware completion timestamps and location-based time handling ... 48e33c67a0

Properties now store a time_zone (IANA string, e.g. "America/Denver") set
automatically when GPS location is detected. Work queue items record
completed_at when checked off, displayed as "Finished at 2:34 PM" in the
property's local timezone. Browser timezone cookie drives Date.current so
"Today" and all date logic respects the crew's actual location instead of
the hardcoded Mountain Time default. Also fixes bin/dev to start docker
compose (the old version required locally installed gems).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-09 01:39:00 +00:00

Author

Owner

Copy link

PR #184 Review

DOMAIN REVIEW

Stack: Ruby on Rails 8.1, Hotwire/Turbo, Stimulus JS, PostgreSQL, Docker-based dev.

Model layer (WorkQueueItem, Property)

• toggle_completion! is clean -- uses update! so failures raise, and atomically pairs completed with completed_at. Good pattern.
• finished_at_local correctly falls back to Time.zone (set from the browser cookie) when the property has no stored timezone. The chain: property timezone -> browser timezone -> app default is sound.
• ActiveSupport::TimeZone[] returns nil for unrecognized strings, so local_time_zone is safe against garbage input. Good.

Controller layer

• set_time_zone_from_browser in ApplicationController reads cookies[:browser_tz] and only sets Time.zone if ActiveSupport::TimeZone[] returns a valid zone. This is safe -- ActiveSupport::TimeZone[] acts as implicit validation; arbitrary cookie values are rejected silently. No injection risk.
• properties_controller.rb: :time_zone correctly added to strong params. The time_zone value comes from Intl.DateTimeFormat().resolvedOptions().timeZone on the client (IANA string like America/Denver), which is appropriate.

View layer

• The inline <script> tag in application.html.erb for setting the browser_tz cookie is minimal and correct. SameSite=Lax and path=/ are appropriate. The 1-year max-age is reasonable for a timezone preference.
• _queue_item.html.erb guards with item.completed? && item.completed_at.present? before calling finished_at_local.strftime(...). This correctly handles the edge case where legacy items might have completed: true but completed_at: nil (from before this migration). Good defensive coding.

JavaScript (add_location_controller.js)

• Storing this.lat / this.lon from the GPS callback and sending them alongside time_zone in the PATCH payload is clean. This fixes a gap where lat/lng were detected but never persisted.

Migration

• Two add_column calls in a single change migration -- simple and reversible. No data migration needed since these are new nullable columns.

bin/dev

• Changed from Ruby exec to bash docker compose up. This is a dev tooling fix, not a feature change. Appropriate for the repo's Docker-based workflow.

BLOCKERS

None.

• Test coverage is present: 5 new model specs for toggle_completion! and finished_at_local, plus 2 updated/new request specs for the toggle endpoint. Both happy path and edge cases (nil completed_at, uncomplete flow) are covered.
• No secrets or credentials in the diff.
• No unvalidated user input -- the browser_tz cookie is validated through ActiveSupport::TimeZone[] before use, and time_zone in strong params is a plain string column with no execution risk.
• No DRY violations in auth paths.

NITS

1. finished_at_local fallback test could be more specific (spec/models/work_queue_item_spec.rb:49-51): The "falls back to app timezone when property has no timezone" test only asserts be_present. It would be stronger to assert the actual timezone name matches Time.zone.name to confirm the fallback logic works correctly, not just that a non-nil value is returned.

2. Cookie value encoding: The inline script does not URI-encode the timezone string before writing it to the cookie. IANA timezone names like America/New_York contain / which is technically safe in cookie values per RFC 6265, but America/Argentina/Buenos_Aires (three-part names) also work fine. This is a non-issue in practice -- just noting for completeness.

3. time_zone column has no length constraint: The string column for time_zone defaults to varchar(255) in PostgreSQL, which is more than sufficient for any IANA timezone name (max ~30 chars). No action needed, but a model-level validates :time_zone, length: { maximum: 50 }, allow_blank: true could add defense-in-depth if desired.

4. No validation that time_zone is a recognized IANA string on write: Currently any string can be stored in properties.time_zone. A model validation like validate { errors.add(:time_zone, "is not recognized") if time_zone.present? && ActiveSupport::TimeZone[time_zone].nil? } would catch bad data at write time rather than silently falling back at read time. Non-blocking since local_time_zone already handles invalid values gracefully.

SOP COMPLIANCE

• Branch named after issue: feature/timezone-aware-completion -- does not follow the {issue-number}-{kebab-case} convention (would be 183-timezone-aware-completion), but this is a nit given the branch clearly relates to the feature.
• PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present and detailed.
• Related references plan slug: No plan slug referenced. Noted as "No plan slug" in the task description, so accepted.
• No secrets committed: Confirmed, no credentials or .env files in the diff.
• No unnecessary file changes: The bin/dev change is a dev tooling fix bundled with the feature -- minor scope expansion but reasonable since it is a one-liner fixing the dev workflow.
• Commit messages: Single commit, descriptive title matches PR.

PROCESS OBSERVATIONS

• Change size: 108 additions / 10 deletions across 13 files is well-scoped. The feature is cohesive: timestamps + timezone storage + browser detection + display.
• Test coverage ratio: Good -- model specs cover the core logic (toggle + timezone conversion), and request specs verify the HTTP layer. The view rendering is covered indirectly through request specs.
• Data migration safety: No backfill needed. Existing completed items will show the address (not "Finished at") until they are toggled again, which is acceptable behavior. The completed? && completed_at.present? guard handles this correctly.
• Deployment risk: Low. New nullable columns, no breaking schema changes, backward-compatible behavior.

VERDICT: APPROVED

## PR #184 Review ### DOMAIN REVIEW **Stack:** Ruby on Rails 8.1, Hotwire/Turbo, Stimulus JS, PostgreSQL, Docker-based dev. **Model layer (`WorkQueueItem`, `Property`)** - `toggle_completion!` is clean -- uses `update!` so failures raise, and atomically pairs `completed` with `completed_at`. Good pattern. - `finished_at_local` correctly falls back to `Time.zone` (set from the browser cookie) when the property has no stored timezone. The chain: property timezone -> browser timezone -> app default is sound. - `ActiveSupport::TimeZone[]` returns `nil` for unrecognized strings, so `local_time_zone` is safe against garbage input. Good. **Controller layer** - `set_time_zone_from_browser` in `ApplicationController` reads `cookies[:browser_tz]` and only sets `Time.zone` if `ActiveSupport::TimeZone[]` returns a valid zone. This is safe -- `ActiveSupport::TimeZone[]` acts as implicit validation; arbitrary cookie values are rejected silently. No injection risk. - `properties_controller.rb`: `:time_zone` correctly added to strong params. The `time_zone` value comes from `Intl.DateTimeFormat().resolvedOptions().timeZone` on the client (IANA string like `America/Denver`), which is appropriate. **View layer** - The inline `<script>` tag in `application.html.erb` for setting the `browser_tz` cookie is minimal and correct. `SameSite=Lax` and `path=/` are appropriate. The 1-year `max-age` is reasonable for a timezone preference. - `_queue_item.html.erb` guards with `item.completed? && item.completed_at.present?` before calling `finished_at_local.strftime(...)`. This correctly handles the edge case where legacy items might have `completed: true` but `completed_at: nil` (from before this migration). Good defensive coding. **JavaScript (`add_location_controller.js`)** - Storing `this.lat` / `this.lon` from the GPS callback and sending them alongside `time_zone` in the PATCH payload is clean. This fixes a gap where lat/lng were detected but never persisted. **Migration** - Two `add_column` calls in a single `change` migration -- simple and reversible. No data migration needed since these are new nullable columns. **`bin/dev`** - Changed from Ruby exec to bash `docker compose up`. This is a dev tooling fix, not a feature change. Appropriate for the repo's Docker-based workflow. ### BLOCKERS None. - Test coverage is present: 5 new model specs for `toggle_completion!` and `finished_at_local`, plus 2 updated/new request specs for the toggle endpoint. Both happy path and edge cases (nil completed_at, uncomplete flow) are covered. - No secrets or credentials in the diff. - No unvalidated user input -- the `browser_tz` cookie is validated through `ActiveSupport::TimeZone[]` before use, and `time_zone` in strong params is a plain string column with no execution risk. - No DRY violations in auth paths. ### NITS 1. **`finished_at_local` fallback test could be more specific (spec/models/work_queue_item_spec.rb:49-51):** The "falls back to app timezone when property has no timezone" test only asserts `be_present`. It would be stronger to assert the actual timezone name matches `Time.zone.name` to confirm the fallback logic works correctly, not just that a non-nil value is returned. 2. **Cookie value encoding:** The inline script does not URI-encode the timezone string before writing it to the cookie. IANA timezone names like `America/New_York` contain `/` which is technically safe in cookie values per RFC 6265, but `America/Argentina/Buenos_Aires` (three-part names) also work fine. This is a non-issue in practice -- just noting for completeness. 3. **`time_zone` column has no length constraint:** The `string` column for `time_zone` defaults to varchar(255) in PostgreSQL, which is more than sufficient for any IANA timezone name (max ~30 chars). No action needed, but a model-level `validates :time_zone, length: { maximum: 50 }, allow_blank: true` could add defense-in-depth if desired. 4. **No validation that `time_zone` is a recognized IANA string on write:** Currently any string can be stored in `properties.time_zone`. A model validation like `validate { errors.add(:time_zone, "is not recognized") if time_zone.present? && ActiveSupport::TimeZone[time_zone].nil? }` would catch bad data at write time rather than silently falling back at read time. Non-blocking since `local_time_zone` already handles invalid values gracefully. ### SOP COMPLIANCE - [x] Branch named after issue: `feature/timezone-aware-completion` -- does not follow the `{issue-number}-{kebab-case}` convention (would be `183-timezone-aware-completion`), but this is a nit given the branch clearly relates to the feature. - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present and detailed. - [ ] Related references plan slug: No plan slug referenced. Noted as "No plan slug" in the task description, so accepted. - [x] No secrets committed: Confirmed, no credentials or .env files in the diff. - [x] No unnecessary file changes: The `bin/dev` change is a dev tooling fix bundled with the feature -- minor scope expansion but reasonable since it is a one-liner fixing the dev workflow. - [x] Commit messages: Single commit, descriptive title matches PR. ### PROCESS OBSERVATIONS - **Change size:** 108 additions / 10 deletions across 13 files is well-scoped. The feature is cohesive: timestamps + timezone storage + browser detection + display. - **Test coverage ratio:** Good -- model specs cover the core logic (toggle + timezone conversion), and request specs verify the HTTP layer. The view rendering is covered indirectly through request specs. - **Data migration safety:** No backfill needed. Existing `completed` items will show the address (not "Finished at") until they are toggled again, which is acceptable behavior. The `completed? && completed_at.present?` guard handles this correctly. - **Deployment risk:** Low. New nullable columns, no breaking schema changes, backward-compatible behavior. ### VERDICT: APPROVED

ldraney added 1 commit 2026-06-09 01:51:28 +00:00

Revert bin/dev change — gems are installed on host ... 48cb6ca59d

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney added 1 commit 2026-06-09 01:55:31 +00:00

Rakefile dev task: docker compose with port 7143 takeover ... 31a3ee2aef

Stops whatever container currently owns port 7143 before starting,
so the dev URL (landscaping-dev.tail5b443a.ts.net) always points to
whichever checkout runs `rake dev`.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney added 1 commit 2026-06-09 01:58:54 +00:00

Fix review nits: timezone validation, stronger test assertions ... a92b1c3c2e

- Add validates :time_zone length and IANA validity on Property
- Assert specific timezone name in fallback spec instead of be_present
- Add property model specs for timezone validation

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-09 01:59:08 +00:00

Author

Owner

Copy link

Nit fixes (post-review)

All 4 nits from QA addressed:

1. Stronger test assertion -- fallback timezone spec now asserts eq("Mountain Time (US & Canada)") instead of be_present
2. Model-level timezone validation -- Property now validates time_zone is a recognized IANA string via ActiveSupport::TimeZone[] lookup
3. Length constraint -- validates :time_zone, length: { maximum: 50 }
4. Branch name -- acknowledged, not renaming mid-PR

Also added:

• Rakefile dev task -- now runs docker compose up on port 7143 with automatic takeover (stops whatever currently owns the port). This makes rake dev the single entry point for local dev, pointing the dev URL (landscaping-dev.tail5b443a.ts.net) at whichever checkout you run it from.
• 3 new property model specs for timezone validation (valid, invalid, blank)

All 19 model specs pass.

## Nit fixes (post-review) All 4 nits from QA addressed: 1. **Stronger test assertion** -- fallback timezone spec now asserts `eq("Mountain Time (US & Canada)")` instead of `be_present` 2. **Model-level timezone validation** -- `Property` now validates `time_zone` is a recognized IANA string via `ActiveSupport::TimeZone[]` lookup 3. **Length constraint** -- `validates :time_zone, length: { maximum: 50 }` 4. **Branch name** -- acknowledged, not renaming mid-PR Also added: - **Rakefile `dev` task** -- now runs `docker compose up` on port 7143 with automatic takeover (stops whatever currently owns the port). This makes `rake dev` the single entry point for local dev, pointing the dev URL (`landscaping-dev.tail5b443a.ts.net`) at whichever checkout you run it from. - **3 new property model specs** for timezone validation (valid, invalid, blank) All 19 model specs pass.

ldraney added 1 commit 2026-06-09 02:01:07 +00:00

bin/setup: check Ruby version and docker compose before proceeding ... e067cb54cb

Fails fast with actionable messages instead of cryptic bundle errors.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney added 1 commit 2026-06-09 02:02:43 +00:00

docs: add local dev setup guide for Arch Linux ... a480498d26

Covers rbenv, docker compose, bin/setup, rake dev, and how
host gems + Docker bind mount fit together.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-09 02:04:59 +00:00

Author

Owner

Copy link

PR #184 Review (Re-review)

Previous review: APPROVED with 4 nits. This re-review covers the nit fixes and additional changes since then.

DOMAIN REVIEW

Stack: Ruby on Rails 8.1, Stimulus JS, ERB templates, RSpec, Docker Compose, PostgreSQL.

Model layer (Property)

• Timezone validation is correctly implemented: validates :time_zone, length: { maximum: 50 }, allow_blank: true combined with the custom time_zone_must_be_valid validator that checks ActiveSupport::TimeZone[time_zone]. The allow_blank: true on length plus if: -> { time_zone.present? } on the custom validator means blank strings pass cleanly and only non-blank values are checked against the IANA registry. Correct.
• local_time_zone helper returns the ActiveSupport::TimeZone object or nil. Clean interface.

Model layer (WorkQueueItem)

• toggle_completion! uses update! which will raise on validation failure -- appropriate for a bang method.
• finished_at_local fallback chain: property timezone -> Time.zone (set by browser cookie or app default). Sound design.

Controller layer

• set_time_zone_from_browser: reads browser_tz cookie, looks it up in ActiveSupport::TimeZone, sets Time.zone only if valid. The ActiveSupport::TimeZone[] lookup acts as implicit validation -- arbitrary cookie strings that are not valid IANA names return nil and are silently ignored. No injection risk.
• :time_zone properly added to strong params in properties_controller.rb.

View layer

• Inline <script> tag for the browser_tz cookie is minimal and correct. SameSite=Lax is appropriate. Max-age of 31536000 (1 year) is reasonable.
• The queue item partial checks both item.completed? AND item.completed_at.present? before calling finished_at_local.strftime, which avoids nil errors for any legacy data that has completed: true but no completed_at.

JavaScript (add_location_controller.js)

• Stores lat/lon on the controller instance before passing to reverseGeocode, then sends all three (latitude, longitude, time_zone) in the PATCH body. Intl.DateTimeFormat().resolvedOptions().timeZone is the standard browser API for IANA timezone detection. Good.

Migration

• Adds completed_at (datetime) and time_zone (string). Both nullable, no default needed. Clean and reversible via change.

Test coverage

• 3 new property model specs (valid IANA, invalid string rejected, blank allowed) -- covers the validation logic.
• 2 new toggle_completion! specs (complete and uncomplete paths).
• 3 new finished_at_local specs (property timezone, fallback to app timezone, nil when not completed).
• 2 updated/new request specs (toggle sets completed_at, uncomplete clears it).
• Fallback timezone test now asserts eq("Mountain Time (US & Canada)") matching config/application.rb line 37. Correct fix from previous nit.
• Total: 10 new or updated test cases for the timezone feature. Coverage is solid.

Rakefile dev task

• Uses docker ps --filter "publish=7143" to find the existing container, then inspects the compose project label and runs docker compose -p #{project} down. One note: the #{existing} and #{project} variables are interpolated into backtick/system calls, but both values come from docker ps and docker inspect output (container IDs and compose project names), not from user input. The risk is minimal in a dev-only Rake task.

bin/setup

• Ruby version check against .ruby-version is a good guard. The docker compose version check prevents cryptic errors later. Both abort with actionable messages.

docs/local-dev-setup.md

• Comprehensive and accurate. Covers prerequisites, first-time setup, running the app, how gems work across host/container boundary, auth setup, and common commands. Good addition.

BLOCKERS

None.

NITS

1. Rakefile shell interpolation (Rakefile line 13): #{existing} is interpolated into a backtick expression. While the value comes from docker ps output and this is a dev-only task, using Open3.capture2 or passing the variable as a separate argument would be more defensive. Non-blocking since the input source is trusted.

2. Cookie encoding (application.html.erb line 24): If the timezone string ever contains characters that need URL encoding (unlikely for IANA names but technically possible for some edge locales), the cookie value is set raw. Consider encodeURIComponent() for defense-in-depth: document.cookie="browser_tz="+encodeURIComponent(Intl.DateTimeFormat().resolvedOptions().timeZone)+.... Non-blocking.

SOP COMPLIANCE

• Branch named after issue: feature/timezone-aware-completion -- descriptive, though convention is {issue-number}-{purpose} (would be 183-timezone-aware-completion). Non-blocking.
• PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related all present and substantive.
• Related references plan slug: No plan slug provided (stated upfront). Acceptable for this PR.
• No secrets committed.
• No unnecessary file changes -- all 16 files are directly relevant to the feature or dev ergonomics.
• Commit messages are descriptive.

PROCESS OBSERVATIONS

• The nit fixes from the previous review are all addressed: timezone validation with IANA check + length limit, specific assertion in fallback test, and the dev tooling improvements (Rakefile port takeover, bin/setup prereq checks, local-dev-setup.md) are clean additions that improve onboarding.
• Test coverage is thorough -- model, request, validation, and edge cases (nil completed_at, blank timezone, invalid timezone) are all covered.
• The toggle_completion! pattern is a good refactor from the previous inline boolean flip in the controller, centralizing the completed/completed_at coupling in the model where it belongs.

VERDICT: APPROVED

## PR #184 Review (Re-review) Previous review: APPROVED with 4 nits. This re-review covers the nit fixes and additional changes since then. ### DOMAIN REVIEW **Stack**: Ruby on Rails 8.1, Stimulus JS, ERB templates, RSpec, Docker Compose, PostgreSQL. **Model layer (Property)** - Timezone validation is correctly implemented: `validates :time_zone, length: { maximum: 50 }, allow_blank: true` combined with the custom `time_zone_must_be_valid` validator that checks `ActiveSupport::TimeZone[time_zone]`. The `allow_blank: true` on length plus `if: -> { time_zone.present? }` on the custom validator means blank strings pass cleanly and only non-blank values are checked against the IANA registry. Correct. - `local_time_zone` helper returns the `ActiveSupport::TimeZone` object or nil. Clean interface. **Model layer (WorkQueueItem)** - `toggle_completion!` uses `update!` which will raise on validation failure -- appropriate for a bang method. - `finished_at_local` fallback chain: property timezone -> `Time.zone` (set by browser cookie or app default). Sound design. **Controller layer** - `set_time_zone_from_browser`: reads `browser_tz` cookie, looks it up in `ActiveSupport::TimeZone`, sets `Time.zone` only if valid. The `ActiveSupport::TimeZone[]` lookup acts as implicit validation -- arbitrary cookie strings that are not valid IANA names return nil and are silently ignored. No injection risk. - `:time_zone` properly added to strong params in `properties_controller.rb`. **View layer** - Inline `<script>` tag for the browser_tz cookie is minimal and correct. `SameSite=Lax` is appropriate. Max-age of 31536000 (1 year) is reasonable. - The queue item partial checks both `item.completed?` AND `item.completed_at.present?` before calling `finished_at_local.strftime`, which avoids nil errors for any legacy data that has `completed: true` but no `completed_at`. **JavaScript (add_location_controller.js)** - Stores lat/lon on the controller instance before passing to `reverseGeocode`, then sends all three (latitude, longitude, time_zone) in the PATCH body. `Intl.DateTimeFormat().resolvedOptions().timeZone` is the standard browser API for IANA timezone detection. Good. **Migration** - Adds `completed_at` (datetime) and `time_zone` (string). Both nullable, no default needed. Clean and reversible via `change`. **Test coverage** - 3 new property model specs (valid IANA, invalid string rejected, blank allowed) -- covers the validation logic. - 2 new `toggle_completion!` specs (complete and uncomplete paths). - 3 new `finished_at_local` specs (property timezone, fallback to app timezone, nil when not completed). - 2 updated/new request specs (toggle sets completed_at, uncomplete clears it). - Fallback timezone test now asserts `eq("Mountain Time (US & Canada)")` matching `config/application.rb` line 37. Correct fix from previous nit. - Total: 10 new or updated test cases for the timezone feature. Coverage is solid. **Rakefile `dev` task** - Uses `docker ps --filter "publish=7143"` to find the existing container, then inspects the compose project label and runs `docker compose -p #{project} down`. One note: the `#{existing}` and `#{project}` variables are interpolated into backtick/system calls, but both values come from `docker ps` and `docker inspect` output (container IDs and compose project names), not from user input. The risk is minimal in a dev-only Rake task. **bin/setup** - Ruby version check against `.ruby-version` is a good guard. The `docker compose version` check prevents cryptic errors later. Both abort with actionable messages. **docs/local-dev-setup.md** - Comprehensive and accurate. Covers prerequisites, first-time setup, running the app, how gems work across host/container boundary, auth setup, and common commands. Good addition. ### BLOCKERS None. ### NITS 1. **Rakefile shell interpolation** (`Rakefile` line 13): `#{existing}` is interpolated into a backtick expression. While the value comes from `docker ps` output and this is a dev-only task, using `Open3.capture2` or passing the variable as a separate argument would be more defensive. Non-blocking since the input source is trusted. 2. **Cookie encoding** (`application.html.erb` line 24): If the timezone string ever contains characters that need URL encoding (unlikely for IANA names but technically possible for some edge locales), the cookie value is set raw. Consider `encodeURIComponent()` for defense-in-depth: `document.cookie="browser_tz="+encodeURIComponent(Intl.DateTimeFormat().resolvedOptions().timeZone)+...`. Non-blocking. ### SOP COMPLIANCE - [x] Branch named after issue: `feature/timezone-aware-completion` -- descriptive, though convention is `{issue-number}-{purpose}` (would be `183-timezone-aware-completion`). Non-blocking. - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related all present and substantive. - [ ] Related references plan slug: No plan slug provided (stated upfront). Acceptable for this PR. - [x] No secrets committed. - [x] No unnecessary file changes -- all 16 files are directly relevant to the feature or dev ergonomics. - [x] Commit messages are descriptive. ### PROCESS OBSERVATIONS - The nit fixes from the previous review are all addressed: timezone validation with IANA check + length limit, specific assertion in fallback test, and the dev tooling improvements (Rakefile port takeover, bin/setup prereq checks, local-dev-setup.md) are clean additions that improve onboarding. - Test coverage is thorough -- model, request, validation, and edge cases (nil completed_at, blank timezone, invalid timezone) are all covered. - The `toggle_completion!` pattern is a good refactor from the previous inline boolean flip in the controller, centralizing the completed/completed_at coupling in the model where it belongs. ### VERDICT: APPROVED

ldraney merged commit ff1fd22f9f into main 2026-06-09 02:08:15 +00:00

ldraney deleted branch feature/timezone-aware-completion 2026-06-09 02:08:15 +00:00

ldraney referenced this pull request from a commit 2026-06-09 02:08:16 +00:00

Timezone-aware completion timestamps and location-based time (#184)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/landscaping-assistant!184

No description provided.