pal-e-docs

Review: Hours tab: clock in/out/break UI

review-1938-2026-07-27 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Board item #1938 — Hours tab: clock in/out/break UI (Forgejo issue #333, 5 points, sprint:7)

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — Related to #331 (parent), depends on #332 (model ticket)
  • [x] Repo — ldraney/landscaping-assistant
  • [x] User Story — As a crew member or lead, clock in/out/break
  • [x] Context — BusyBusy interaction model described
  • [x] File Targets — 2 create, 5 modify, exclusions listed
  • [x] Feature Flag — time_tracking (registered by model ticket)
  • [x] Acceptance Criteria — 10 items
  • [x] Test Expectations — 5 items + run command
  • [x] Constraints — 4 items
  • [x] Checklist — standard
  • [x] Related — project + parent
All template sections present and complete.

Traceability

  • [x] story:crew-time-tracking label — present on board item
  • [ ] story note MISSING — [SCOPE] No "crew-time-tracking" entry in project-landscaping-assistant user-stories section. Must be added before work begins.
  • [x] arch:rails label — present on board item
  • [ ] arch note MISMATCH — [SCOPE] arch-rails note exists but describes ISS Rails Architecture (project: prediction-assistant). It does not document landscaping-assistant’s Rails architecture. Either update arch-rails to be multi-project or create a project-specific note.
  • [x] Forgejo issue — #333, open, valid URL

File Targets

  • [x] app/views/hours/index.html.erb — TO CREATE. Directory does not exist; expected for greenfield.
  • [x] app/javascript/controllers/timer_controller.js — TO CREATE. Controllers directory exists with 19 Stimulus controllers. Uses eagerLoadControllersFrom so no index.js changes needed.
  • [x] app/views/layouts/application.html.erb — verified exists. Nav uses 5-slot grid (nav-slot-1 through nav-slot-5). Slot 2 is currently Week (admin-only). Hours would reuse slot 2 for member/lead since Week and Hours are mutually exclusive by role.
  • [x] app/controllers/application_controller.rb — verified exists. TAB_ROLES at line 132 has: today, week, profile, properties, crew. Adding :hours for member/lead is straightforward.
  • [ ] app/controllers/hours_controller.rb — ISSUE: [BODY] Listed as "Files to modify" but does NOT exist. Model ticket #332 will CREATE this file. Should be listed under a "Files to modify (created by #332)" sub-heading or include a parenthetical note.
  • [x] app/assets/stylesheets/application.css — verified exists (59KB).
  • [x] config/routes.rb — verified exists. No hours routes present; ticket correctly notes "may already be added by model ticket."

Repo Placement

Correct. All work is in ldraney/landscaping-assistant. Forgejo issue filed on the same repo. No cross-repo scope.

Dependencies

  • #332 (TimeEntry data model + API + flag) — sprint:6, backlog. Hard dependency. Creates the controller, models, routes, and feature flag that #333 consumes. Sprint ordering (6 before 7) is correct. Dependency is documented in Lineage section.
  • #331 (Time Tracking: Hours Tab) — parent umbrella, backlog. No blocking dependency.
  • #334 (Hours tab: daily/weekly summary) — sprint:7, backlog. Sibling ticket. Independent — can run in parallel with #333 since it adds separate views to the same tab.
  • No items in in_progress or needs_approval block this ticket. #1940 (Keycloak redirect bug) is in_progress but unrelated.

Acceptance Criteria

10 acceptance criteria. Testability assessment:
  • AC 1-2 (tab visibility by role): Testable via request specs. Covered by test expectations.
  • AC 3 (Clock In creates entry): Testable via request spec. Covered.
  • AC 4 (Timer displays HH:MM:SS, updates every second): Client-side JS only. Not covered by any test expectation. [BODY] Add Stimulus controller test or note that this is manually verified.
  • AC 5 (Take Break pauses timer): Client-side JS only. Not covered by test expectations.
  • AC 6 (End Break resumes timer): Client-side JS only. Not covered by test expectations.
  • AC 7 (Clock Out stops timer): Mix of server + client. Server side covered.
  • AC 8 (Today's log shows entries): Testable via request spec content check. Not explicitly covered but implied.
  • AC 9 (Timer survives page refresh): Testable by verifying server response contains reconstruct data. Not explicitly covered.
  • AC 10 (Tab hidden when flag disabled): Covered by test expectations.
Gap: AC 4-6 are pure Stimulus controller behavior with no test coverage specified. Either add JS test expectations or note manual verification.

Blast Radius

No existing time tracking code in the codebase (grep confirmed). This is a clean greenfield addition. The nav slot 2 reuse (Week for admin, Hours for member/lead) is architecturally sound since the roles are mutually exclusive, but should be documented via a code comment. No downstream consumers affected.

Decomposition Assessment

Exceeds threshold.
  • File targets: 7 (2 create + 5 modify), all in 1 repo — OK
  • Acceptance criteria: 10 (threshold: >5) — EXCEEDS
  • Estimated agent work: Borderline 5 minutes. Creating nav tab entry, view, Stimulus controller, CSS, and request specs. The AC items are interrelated (all states of one widget), so the work is cohesive.
NEEDS DECOMPOSITION — route to skill-decompose-ticket. Suggested split:
  • Hours tab: nav + route + page shell (AC 1, 2, 10) — Add :hours to TAB_ROLES, nav slot 2, basic index view, flag gating. ~2 points.
  • Hours tab: timer widget + clock/break UI (AC 3-9) — Stimulus timer_controller.js, button states, Turbo Stream actions, today’s log. ~3 points.

Recommendation

  • [SCOPE] Create user story entry "crew-time-tracking" on project-landscaping-assistant user-stories section.
  • [SCOPE] arch-rails note describes ISS Rails, not landscaping-assistant. Update arch-rails to be multi-project or create a landscaping-assistant-specific variant.
  • [BODY] Clarify app/controllers/hours_controller.rb is created by #332, not pre-existing. Move from "Files to modify" to a note like "(created by dependency #332)".
  • [BODY] Add test expectations for Stimulus controller behavior (AC 4-6: timer display, break pause/resume). Currently only request specs are specified but three AC are pure client-side JS.
  • [DECOMPOSE] 10 AC across 7 file targets exceeds the 5-minute rule. Split into nav/page-shell ticket and timer-widget ticket. Route to skill-decompose-ticket.