Add weekly tracking view, properties tab, and four-tab nav #13
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "10-weekly-tracking"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Changes
app/controllers/weeks_controller.rb: new controller aggregating weekly completionsapp/views/weeks/index.html.erb: week grid with day columns, checkmarks, prev/next nav, summaryapp/controllers/properties_controller.rb: manage and toggle_active actionsapp/views/properties/manage.html.erb: property list with search and active/inactive toggleapp/views/properties/toggle_active.turbo_stream.erb: no-reload toggle responseapp/views/layouts/application.html.erb: four-tab nav (Today, Week, New, Properties)config/routes.rb: weeks resource, manage/toggle_active routes, root to todayapp/assets/stylesheets/application.css: week grid, manage list, toggle button, inactive dimmingdb/schema.rb: version bump for active columnspec/requests/weeks_spec.rb: 8 request specsspec/requests/properties_spec.rb: 6 request specsTest Plan
Review Checklist
Related Notes
project-landscaping-assistantPR #13 Review
DOMAIN REVIEW
Tech stack: Ruby on Rails 8.1, Hotwire (Turbo Streams/Frames), Stimulus, vanilla CSS with design tokens.
WeeksController query correctness (
app/controllers/weeks_controller.rb):[property_id, work_date], groups by property_id, then extracts dates. This is correct and efficient -- one query, no N+1.beginning_of_week(:monday)consistently and rescuesDate::Errorfor invalid params. Solid.@total_completed(line 15-16): The ternary checks ifflatten.uniq.length > 0then returns@completions.keys.count. This counts "properties that had at least one completion" but theuniqblock with{ |d| d }is redundant (equivalent to plain.uniq). The logic works but is confusingly written -- it would be clearer as@completions.keys.countalone since@completionsonly contains completed items anyway (the hash is empty if nothing was completed).Turbo Stream toggle_active -- BROKEN after first toggle:
In
manage.html.erb(line 11), each property is wrapped:This renders as
<turbo-frame id="manage_property_123"><li>...</li></turbo-frame>.In
toggle_active.turbo_stream.erb(line 1), the replacement targets that same ID:The replacement content is a bare
<li>-- it does NOT re-wrap in a<turbo-frame>tag. After the first toggle,turbo_stream.replaceremoves the entire<turbo-frame id="manage_property_123">element and inserts a bare<li>. On the second toggle click, Turbo cannot find an element withid="manage_property_123"because it no longer exists in the DOM. The second and all subsequent toggles will silently fail (no error, no UI update -- the Turbo Stream response just has nowhere to land).Fix: Wrap the replacement
<li>in a matchingturbo_frame_tagin the turbo stream template, or switch to using a plainidattribute on the<li>itself (removing the turbo-frame wrapper in both files and relying on adom_id-basedidattribute on the<li>directly).Four-tab nav active state logic (
app/views/layouts/application.html.erb):controller_name == 'work_queue_items'-- correct.controller_name == 'weeks'-- correct.controller_name == 'properties' && action_name == 'index'-- correct.controller_name == 'properties' && action_name == 'manage'-- correct.showandupdateactions on PropertiesController will have no active tab highlighted. Acceptable for now since show is accessed contextually.CSS design token usage: All colors, spacing, radii, and borders use
var(--color-*),var(--spacing-*), andvar(--radius)tokens. No hardcoded color values. Oneborder-radius: 1remon.btn-toggleis a reasonable pill-shape choice outside the token system. Clean.Route structure: Well-organized.
manageas collection route,toggle_activeas member route with PATCH -- RESTful and correct.BLOCKERS
app/views/properties/toggle_active.turbo_stream.erb): The replacement content lacks the<turbo-frame>wrapper present in the initial render. After one toggle, subsequent toggles silently fail -- the UI becomes unresponsive for that property until page reload. This is a functional bug in core feature behavior.NITS
WeeksController
@total_completedlogic is convoluted (line 15-16): The ternary with.uniq { |d| d }is equivalent to.uniqand the length check is unnecessary since an empty hash would return 0 from.keys.countanyway. Simplify to@total_completed = @completions.keys.count.DRY opportunity in manage templates: The
<li>markup inmanage.html.erbandtoggle_active.turbo_stream.erbis duplicated. Consider extracting to a partial (_manage_item.html.erb) rendered in both places. Not blocking since these are small and unlikely to diverge for now, but worth noting.Missing
aria-labelon toggle buttons: The "Active"/"Inactive" text is inside the button so screen readers get it, but addingaria-pressedwould improve accessibility semantics for toggle buttons.@propertiesin WeeksController triggers N+1 forclient_name: CurrentlyProperty.active.order(:address_line)-- sinceclient_nameis on the same table this is fine, but if services were ever displayed in the week view, an.includes(:services)would be needed. Not an issue today.SOP COMPLIANCE
10-weekly-trackingmatches issue #10project-landscaping-assistantreferencedPROCESS OBSERVATIONS
VERDICT: NOT APPROVED
One blocker: the Turbo Stream replace target is destroyed after first toggle, making the toggle_active feature break on repeated use. Fix the turbo stream template to preserve the frame wrapper (or switch to ID-based targeting without frames), and this is ready to merge.
PR #13 Review
DOMAIN REVIEW
Tech Stack: Ruby on Rails 8.1, Hotwire (Turbo Streams + Stimulus), PostgreSQL, RSpec
Turbo Stream Blocker Fix (Verified)
The previous review's blocker is resolved. Both
manage.html.erbandtoggle_active.turbo_stream.erbnow use the same DOM structure:No
turbo_frame_tagwrapper in either file.turbo_stream.replacetargets theidattribute directly, so subsequent toggles will always find the element. Correct fix.WeeksController
parse_week_startproperly rescuesDate::Errorfor invalid params -- good defensive coding.indexuses.pluck(:property_id, :work_date)which avoids loading full AR objects -- efficient.@total_completed = @completions.keys.countis clean and correct (counts distinct properties with at least one completion that week).PropertiesController#toggle_active
update!which will raise on validation failure -- appropriate for a simple boolean toggle.turbo_streamandhtmlformats -- graceful degradation.Routes
work_queue_items#index(Today queue) -- makes sense as primary use case.CSS
--color-*,--spacing-*,--radius) consistently.2remday columns.Filter Controller (Stimulus)
propertyelements. Thedata-filter-target="property"in both manage and turbo_stream templates ensures search continues working after toggle.BLOCKERS
None. The previous blocker (Turbo Stream replacing a
turbo_frame_tag-wrapped element with a bare<li>) is resolved.NITS
DRY opportunity in templates: The
<li>markup inmanage.html.erbandtoggle_active.turbo_stream.erbis identical (17 lines duplicated). Consider extracting to a partial (_manage_item.html.erb). Not a blocker since this is view markup, not auth/security logic, but reduces maintenance surface.N+1 potential in WeeksController:
Property.active.order(:address_line)does not.includes(:services)-- not needed here since the week view only displaysaddress_lineandclient_name. Fine as-is but worth noting if the view template ever adds service info.total_completededge case:@completions.keys.countcounts properties that had ANY completion in the week, not total completions. The view shows "X/Y completed" where Y is total active properties. The semantics are "properties serviced at least once this week" vs "total active" -- this is reasonable but could be confusing if a property is serviced multiple days. A comment clarifying intent would help.Missing
aria-labelon toggle button: Thebtn-togglebutton says "Active" or "Inactive" as text content, which is fine for screen readers. However, thebutton_togenerates a form, and the button lacks context about which property it toggles. Consider addingaria-label="Toggle active status for #{property.address_line}".SOP COMPLIANCE
10-weekly-trackingmatches issue #10project-landscaping-assistantreferencedPROCESS OBSERVATIONS
VERDICT: APPROVED